- Notifications
You must be signed in to change notification settings - Fork291
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Allow to apply arbitrary plugins when compiling ServiceWorker script#358
base:master
Are you sure you want to change the base?
Conversation
Yeah, please, write tests. |
| @@ -30,6 +30,11 @@ export default class OfflinePlugin { | |||
| AppCache: false | |||
| }); | |||
| if (options.ServiceWorker && options.ServiceWorker.plugins) { | |||
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 done inServiceWorker.js file
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.
It cannot be because it refers tooptions, notthis.options.
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.
That sounds wrong.ServiceWorker options shouldn't be touched inindex.js, they all are handled inServiceWorker.js file.
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 have to clarify a bit more. It usesdeepExtend to set upthis.options, andaddTool passes it toServiceWorker.js. However,deepExtend breaksthis.options.ServiceWorker.plugins andServiceWorker.js can do nothing with it when received. Therefore it has to be fixed beforehand.
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.
Yeah, I see what you mean now. It should be handled in some other way though. I'll merge this and do the change to this part myself.
(tests seem to fail now because master has been updated)
I'll be happy to merge this once everything is fixed. |
I'd like to use this feature to embed some environment variables in my custom entry module with
EnvironmentPlugin. I have optionalCDN_HOSTenvironment variable to determine an object storage in use. The entry module should have the variable, and be able to cache files served from the host.There could be other use cases, considering the wide use of
process.env.NODE_ENV. It is nice to have.Question: I have not implemented a test case, but should I? Should I place it at
tests/legacyortests? (I'm not sure whatlegacymeans.)