Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Fix dynamic-import-chunkname validation regex #1411

Closed
wants to merge 1 commit into from
Closed

[WIP] Fix dynamic-import-chunkname validation regex #1411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/rules/dynamic-import-chunkname.js
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@ module.exports = {
const { webpackChunknameFormat = '[0-9a-zA-Z-_/.]+' } = config || {}

const paddedCommentRegex = /^ (\S[\s\S]+\S) $/
const commentStyleRegex = /^( \w+: ("[^"]*"|\d+|false|true),?)+ $/
const commentStyleRegex = /^( \w+: ("[^"]*"|\/.*\/|\d+|false|true),?)+ $/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not just allow "anything", let's be explicit about webpackInclude/webpackExclude.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not just allow anything. We allow anything between slashes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to be explicit about webpackInclude/webpackExclude then we need to be explicit about webpackPreload + true/false too for example. In this case we need to write more sophisticated parser of magic comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point, and reasonable to say it's out of scope for this PR - but i'd expect that even if the stuff after the : is "anything", that the part before it would have to be more explicit.

Copy link
Author

@vkrol vkrol Jul 8, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be the appropriate solution for now?

-    const commentStyleRegex = /^( \w+: ("[^"]*"|\/.*\/|\d+|false|true),?)+ $/
+    const commentStyleRegex = /^( (webpackChunkName|webpackInclude|webpackExclude|...): ("[^"]*"|\/.*\/|\d+|false|true),?)+ $/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great to me - does that mirror what webpack supports? (like, is the space after the colon required, is the space between /* and w required or optional, etc)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like, is the space after the colon required, is the space between /* and w required or optional, etc)

The space after the colon and the space between /* and w are optional. Do we need to change the regex in this PR or it is out of scope?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems better to change it in this PR, so that after this PR lands, it actually matches what webpack supports and no more.

const chunkSubstrFormat = ` webpackChunkName: "${webpackChunknameFormat}",? `
const chunkSubstrRegex = new RegExp(chunkSubstrFormat)

Expand Down
24 changes: 24 additions & 0 deletions tests/src/rules/dynamic-import-chunkname.js
View file
Open in desktop
Expand Up @@ -32,6 +32,30 @@ ruleTester.run('dynamic-import-chunkname', rule, {
)`,
options,
},
{
code: `dynamicImport(
/* webpackInclude: /some-regex/ */
/* webpackChunkName: "someModule" */
vkrol marked this conversation as resolved.
Show resolved Hide resolved
'test'
)`,
options,
},
{
code: `dynamicImport(
/* webpackChunkName: "someModule" */
/* webpackInclude: /some-regex/ */
'test'
)`,
options,
},
{
code: `dynamicImport(
/* webpackExclude: /some-regex/ */
/* webpackChunkName: "someModule" */
'test'
)`,
options,
},
{
code: `dynamicImport(
/* webpackChunkName: "Some_Other_Module" */
Expand Down