-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Tapable.plugin warning and supports assetPrefix in register-sw #78
Conversation
…er the sw also when assetPrefix has been used
Wow awesome! Should be able to take a look either today or tomorrow! 🥂 |
let content = await readFile(require.resolve('./register-sw.js'), 'utf8'); | ||
content = content.replace('{REGISTER_SW_PREFIX}', registerSwPrefix); | ||
|
||
await writeFile(join(__dirname, 'register-sw-compiled.js'), content, 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not certain about this block because service workers must be served on the origin domain they're being used on.
So, the service worker on jackhanford.com would never register if it was being served on cdn.foobar.com.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, disregard ^ if this is for serving the service worker on a different path on the same domain. ie jackhanford.com/foo/bar/service-worker.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanford yes it's actually to load it from a different path in case your application resides on a virtual directory
eg. my application is on test.com/food/bar
but its trying to get the service worker from test.com/
; this because the register script fetch the service-worker from the root folder.
plugin.js
Outdated
} else { | ||
compiler.plugin( | ||
'done', | ||
async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think if we did async () => generateNextManifest(this.opts)
the await is inferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used the previous syntax but I think it should be ok like that as well
Gonna try this out
plugin.js
Outdated
compiler.hooks.done.tap( | ||
'CopyPlugin', | ||
async () => { | ||
await generateNextManifest(this.opts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with the comment above
if (compiler.hooks) { | ||
compiler.hooks.done.tap( | ||
'CopyPlugin', | ||
async() => generateNextManifest(this.opts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be ok now, tested and seems working well
Thanks, I’ll get this merged and released tomorrow morning! |
@hanford thanks you for the fast feedback 🥂 |
@mirgj Really happy about this PR. The code works, I might tweak it a bit but overall this is good. I am a little worried about doing this to every Do you think this could warrant a new option so we don't need to make that assumption?
cc @joaovieira have any thoughts? |
@hanford I support another property which is Anyway, honestly, I think whoever uses |
@mirgj my hesitation with supporting as I think a lot of people are probably using I'll get this merged and update the option to Thanks for the contribution! 🙌 Is it possible to serve service workers from CDN/remote origin? |
|
What's new
compiler.plugin(...)
is deprecated and gives a warning during the next.js build; added incompiler.hooks.done
as it's the suggested syntax.assetPrefix
supports has if you are fetching your assets from a CDN or virtual directoryregister-sw.js
will always use the root folder to get the service-worker. With this fix will take eitherassetPrefix
(if specified) orregisterSwPrefix
(in case we want to overwriteassetPrefix
value)