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.

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 dynamic-import-chunkname validation regex #1411

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@vkrol
Copy link

commented Jul 8, 2019

Fixes #1407.

@coveralls

This comment has been minimized.

Copy link

commented Jul 8, 2019

Coverage Status

Coverage remained the same at 97.753% when pulling 289e783 on vkrol:fix-dynamic-import-chunkname-validation-regex into 6512110 on benmosher:master.

Show resolved Hide resolved tests/src/rules/dynamic-import-chunkname.js
@@ -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),?)+ $/

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 8, 2019

Collaborator

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

This comment has been minimized.

Copy link
@vkrol

vkrol Jul 8, 2019

Author

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

This comment has been minimized.

Copy link
@vkrol

vkrol Jul 8, 2019

Author

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.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 8, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@vkrol

vkrol Jul 8, 2019

Author

Would this be the appropriate solution for now?

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

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 8, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@vkrol

vkrol Jul 9, 2019

Author

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?

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 12, 2019

Collaborator

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

@vkrol vkrol force-pushed the vkrol:fix-dynamic-import-chunkname-validation-regex branch from 1d388fa to 289e783 Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.