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

feat: add webpackFetchPriority. #18

Merged
merged 6 commits into from
Jun 22, 2023
Merged

feat: add webpackFetchPriority. #18

merged 6 commits into from
Jun 22, 2023

Conversation

morganney
Copy link
Owner

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (aaabb45) 100.00% compared to head (3e4841c) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #18   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        17    +2     
  Lines          210       207    -3     
  Branches        73        71    -2     
=========================================
- Hits           210       207    -3     
Impacted Files Coverage Δ
src/booleanComment.js 100.00% <ø> (ø)
src/schema.js 100.00% <ø> (ø)
src/strategy.js 100.00% <ø> (ø)
src/webpackExports.js 100.00% <ø> (ø)
src/comment.js 100.00% <100.00%> (ø)
src/enumComment.js 100.00% <100.00%> (ø)
src/loader.js 100.00% <100.00%> (ø)
src/regexComment.js 100.00% <100.00%> (ø)
src/util.js 100.00% <100.00%> (ø)
src/webpackFetchPriority.js 100.00% <100.00%> (ø)
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

]
},
"test": {
"plugins": ["babel-plugin-istanbul"]
Copy link
Owner Author

Choose a reason for hiding this comment

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

@@ -0,0 +1 @@
node >= 12.17
Copy link
Owner Author

Choose a reason for hiding this comment

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

Because this version unflags --experimental-modules thereby enabling support for package.json type and exports fields by default.

"engines": {
"node": ">=10.3.0"
"node": ">=12.17.0"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Same reason as this.

Comment on lines -13 to -15
if (option) {
return commentFor[key](filepath, bareImportPath, option, match)
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

The use of schema-utils and validating the options negates the need for this check, thereby allowing the code to be simplified.

additionalProperties: false
}
const schema = {
oneOf: [
{ instanceof: 'Function' },
{ instanceof: 'RegExp' },
Copy link
Owner Author

Choose a reason for hiding this comment

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

This was always intended, just overlooked.

@@ -57,7 +57,7 @@ const getOverrideConfig = (overrides, filepath, config) => {
}
const importPrefix = /^(?:(\.{1,2}\/)+)|^\/|^.+:\/\/\/?[.-\w]+\//
const dynamicImportsWithoutComments =
/(?<![\w.]|#!|\*[\s\w]*?|\/\/\s*|['"`][^)$]*)import\s*\((?!\s*\/\*)(?<path>\s*?['"`][^)]+['"`]\s*)\)(?!\s*?\*\/)/g
/(?<![\w.]|#!|(?:\/{2}.+\n?)+|\/\*[\s\w]*?|\*.+?|['"`][^)$,\n]*)import\s*\((?!\s*\/\*)(?<path>\s*?['"`][^)]+['"`]\s*)\)(?!\s*?\*\/)/gm
Copy link
Owner Author

Choose a reason for hiding this comment

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

I should create a milestone outlining a transition to a JS parser like acorn.

For now, this regex has two known limitations:

  • Multiline comments that are not of the JSDoc style (i.e. started with /* and ended with */) and include more thane one dynamic import string will be incorrectly targeted and the build will break from a syntax error. Multiline comments of the JSDoc style do not ever break the build regardless of how many import strings are present.
    breaks
    /*
       This comment style will break the build if it
       spans more than one line and includes import('module')
       strings more than once. Like import('module2').
    */
    works
    /*
      If you do not include more than one import('module') string,
      then you can have multiline comments of this style without
      breaking the build.
    */
  • Strings that include a dynamic import that are not part of an embedded expression in a template literal, and preceded by a ) or , will be incorrectly targeted and the dynamic import substring will be updated with any configured comments.
    breaks
    const myString1 = '(hi)import("foo/module")(world)' // The ) in (hi) causes this string to be targeted
    const myString2 = '(hi,import("foo/module")(world)' // The , in (hi, causes this string to be targeted
    works
    // Inside an embedded expression things work as expected
    const myString1 = '(hi)${import("foo/module")}(world)'
    const myString2 = '(hi,${import("foo/module")}(world)'

The former case is more likely than the latter, however, both are edge cases that are probably very rare. Either way, pulling in a parser would resolve these issues for good.

@morganney morganney merged commit ad5fc03 into main Jun 22, 2023
3 checks passed
@morganney morganney deleted the develop branch June 22, 2023 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant