Dynamic import chunkname rule #1070
Conversation
|
|
||
| This rule reports any dynamic imports without a webpackChunkName specified in a leading block comment in the proper format. | ||
|
|
||
| This is a useful rule because if the webpackChunkName is not defined in a dynamic import, Webpack will autogenerate the chunk name. |
There was a problem hiding this comment.
This would be better if it explained why it is better to have control over the chunk name vs letting webpack autogenerate one. Basically mention that autogenerated chunk names are not stable across builds, which prevents long-term browser caching.
nit: "webpack" should be lowercase everywhere
| This is a useful rule because if the webpackChunkName is not defined in a dynamic import, Webpack will autogenerate the chunk name. | ||
|
|
||
| ## Rule Details | ||
| This rule runs against `import` by default, but can be configured to also run against an alternative dynamic-import function, e.g. 'dynamicImport.' |
There was a problem hiding this comment.
import here should probably be import() to differentiate it as a dynamic import.
There was a problem hiding this comment.
+1, this needs to be import() since it’s distinct from import
|
|
||
| ## Rule Details | ||
| This rule runs against `import` by default, but can be configured to also run against an alternative dynamic-import function, e.g. 'dynamicImport.' | ||
| You can also configure the regex format you'd like to accept for the webpackChunkName - for example, we don't want the number 6 to show up in our chunk names. |
There was a problem hiding this comment.
"for example, we don't" should maybe be "for example, if we don't". I'd probably also end this sentence with a : instead of a .
| ```javascript | ||
| { | ||
| "dynamic-import-chunkname": [2, { | ||
| importFunction: "dynamicImport", |
There was a problem hiding this comment.
I wonder if this should be importFunctions and take an array instead of a string. What do you think?
| This is a useful rule because if the webpackChunkName is not defined in a dynamic import, Webpack will autogenerate the chunk name. | ||
|
|
||
| ## Rule Details | ||
| This rule runs against `import` by default, but can be configured to also run against an alternative dynamic-import function, e.g. 'dynamicImport.' |
There was a problem hiding this comment.
+1, this needs to be import() since it’s distinct from import
|
|
||
| create: function (context) { | ||
| const config = context.options[0] | ||
| let importFunction |
There was a problem hiding this comment.
instead of a let here, what about const { importFunction } = config || {}?
| ({ importFunction } = config) | ||
| } | ||
|
|
||
| let webpackChunknameFormat = '[0-9a-zA-Z-_/.]+' |
| const commentRegex = new RegExp(commentFormat) | ||
|
|
||
| return { | ||
| CallExpression(node) { |
There was a problem hiding this comment.
Could this be something like:
[`CallExpression[callee.name=“${importFunction}”],CallExpression[callee.type=“Import”]`](node) {?
This might improve linting performance and would allow you to re,over the “if” on line 37
| importFunction: { | ||
| type: 'string', | ||
| importFunctions: { | ||
| type: 'array', |
There was a problem hiding this comment.
can we add uniqueItems: true to this array?
| return { | ||
| [`CallExpression[callee.type="Import"],CallExpression[callee.name]`](node) { | ||
| const { callee: { name }} = node | ||
| if (name && !importFunctions.includes(name)) { |
There was a problem hiding this comment.
i'm not sure if we can use this in node 4, which eslint supports. you may want to do new Set(importFunctions) and then use .has?
…e import functions
| const commentRegex = new RegExp(commentFormat) | ||
|
|
||
| return { | ||
| [`CallExpression[callee.type="Import"],CallExpression[callee.name]`](node) { |
There was a problem hiding this comment.
@ljharb, let me know if you know of a better way to write these selectors now that we're accepting an array of import functions instead of just a string
There was a problem hiding this comment.
hmm - i don't think it has to be a blocker or anything, but you could, perhaps, dynamically construct the method name like so:
const selector = ['CallExpression[callee.type="Import"]'].concat(importFunctions.map(func => `CallExpression[callee.name="${func}"]`).join(',');
return {
[selector](node) {but at this point I'm not sure if the readability hit is worth the performance optimization. What do you think?
There was a problem hiding this comment.
I think this may be what's failing the Travis build for eslint version 2
There was a problem hiding this comment.
oh, snap - good call. if eslint 2 doesn't have full AST selectors, then we should go back to your original code that just checks CallExpression and does the filtering inside the method body :-(
There was a problem hiding this comment.
This is great! Once tests are passing (and pending #1070 (comment), if that's what we decide to do) then this is ready to go.
This PR adds a new rule
dynamic-import-chunkname,which reports on any dynamic-import usages without a properly formatted webpackChunkName leading comment as mentioned in this tweet.This is a useful rule to have, since explicitly defining the webpackChunkName will provide a consistent file name for the code split boundary.
cc @lencioni @ljharb