[WIP] Fix dynamic-import-chunkname validation regex #1411
Conversation
| @@ -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),?)+ $/ | |||
There was a problem hiding this comment.
let's not just allow "anything", let's be explicit about webpackInclude/webpackExclude.
There was a problem hiding this comment.
We do not just allow anything. We allow anything between slashes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would this be the appropriate solution for now?
- const commentStyleRegex = /^( \w+: ("[^"]*"|\/.*\/|\d+|false|true),?)+ $/
+ const commentStyleRegex = /^( (webpackChunkName|webpackInclude|webpackExclude|...): ("[^"]*"|\/.*\/|\d+|false|true),?)+ $/
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
changed the title
|
@vkrol are you still interested in completing this PR? |
|
@ljharb Yes. |
|
Obsoleted by #2330. |
|
That hasn’t landed yet tho. |
|
This was fixed in #2330. |
Fixes #1407.