-
Notifications
You must be signed in to change notification settings - Fork 105
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
Added [chunkhash]/[hash]/[id]/[name] support for importScripts #70
Conversation
@goldhand hmm.. any issue with this? Was hoping to move this forward soon. |
@hulkish nope! I was away this weekend. Glancing over this just now, looks great! Will look a little more in depth today before I merge |
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.
Looks good overall, a couple changes, mostly to the readme. Also, just to clarify, will importScripts: ['myscript.[hash].js']
still be supported or will they need to use the format importScripts: {filename:
myscript.[hash].js'}` to get the legacy hash support
README.md
Outdated
- When importScripts array item is a `String`: | ||
- Converts to object format `{ filename: '<publicPath>/my-script.js'}` | ||
- When importScripts array item is an `Object`: | ||
- Looks for |
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 you for got finish a thought here
README.md
Outdated
@@ -1,4 +1,4 @@ | |||
SW Precache Webpack Plugin | |||
# SW Precache Webpack Plugin |
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.
Need to remove the extra lines under all the titles that were reformatted
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.
Done.
README.md
Outdated
] | ||
``` | ||
|
||
### `importSripts` usage example | ||
Uses one option from `sw-precache` (`cacheId`) with one option from `SWPrecacheWebpackPlugin` (`filename`) in a configuration hash: |
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 doesn't explain the example
README.md
Outdated
] | ||
``` | ||
|
||
Here's a more elaborate example with `mergeStaticsConfig: true` and `staticFileGlobsIgnorePatterns`. `mergeStaticsConfig: true` allows you to add some additional static file globs to the emitted ServiceWorker file alongside Webpack's emitted assets. `staticFileGlobsIgnorePatterns` can be used to avoid including sourcemap file references in the generated ServiceWorker. | ||
### `cacheId` usage example | ||
Uses one option from `sw-precache` (`cacheId`) with one option from `SWPrecacheWebpackPlugin` (`filename`) in a configuration hash: |
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.
Probably can remove the cacheId example
const chunk = chunks.find(c => c.names.includes(criteria.chunkName)); | ||
|
||
if (!chunk) { | ||
compilation.errors.push(new Error( |
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.
Maybe push this to this.warnings
?
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 this should report an error, because it can only happen if they specify a chunk name which does not exist.
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 is the only outstanding requested change, I can change it to warning if you insist - but I believe this should prompt the user to remove their definition of invalid chunkName
.
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.
Ok sounds good, we can leave it
@goldhand I think it should be deprecated in a later release. However, yes - the current usage is legacy supported: importScripts: ['myscript.[hash].js'] The exact same can also be accomplished using new syntax: importScripts: [{ filename: 'myscript.[hash].js' }] Additional Notes:
module.exports = function() {
return import(/* webpackChunkName: "service-worker-imported-script-2" */ './service-worker-imported-script-2');
}; Then, in your importScripts config you can directly mention this named chunk: importScripts: [{ chunkName: 'service-worker-imported-script-2' }] I added tests for this, also. |
@goldhand How's that? |
README.md
Outdated
@@ -1,4 +1,4 @@ | |||
SW Precache Webpack Plugin | |||
# SW Precache Webpack Plugin | |||
========================== |
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.
forgot this one
@hulkish almost there! Just forgot the |
@goldhand The line is crushed! Also, updated title of this PR to more accurately represent what this offers. |
How is the
If I change either the filename or content of
|
For documentation on how this works, please see my changes to README.md.
This satisfies #60