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: allow modules.auto to be a filter function #1086

Merged
merged 1 commit into from
Jun 3, 2020
Merged

feat: allow modules.auto to be a filter function #1086

merged 1 commit into from
Jun 3, 2020

Conversation

BPScott
Copy link
Contributor

@BPScott BPScott commented May 28, 2020

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

The modules.auto feature as added in #1067 is pretty sweet. but sometimes you need more than a simple regex to check against your file path.

This PR allows modules.auto to be a function that that accepts the resourcePath and returns a boolean.

Along the way I tweaked some test case names to be a little clearer about what gets passed into modules.auto, which causes a bit of snapshot diff noise.

Breaking Changes

n/a

Additional Info

n/a

@jsf-clabot
Copy link

jsf-clabot commented May 28, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #1086 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   97.28%   97.29%   +0.01%     
==========================================
  Files          10       10              
  Lines         479      481       +2     
  Branches      161      162       +1     
==========================================
+ Hits          466      468       +2     
  Misses         12       12              
  Partials        1        1              
Impacted Files Coverage Δ
src/utils.js 98.89% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 505d2e6...a28b032. Read the comment docs.

@@ -117,6 +117,10 @@ function shouldUseModulesPlugins(modules, resourcePath) {
return modules.auto.test(resourcePath);
}

if (modules.auto instanceof Function) {
return modules.auto(resourcePath);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use typeof

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good work, please add test to validate-options.test.js too

@BPScott
Copy link
Contributor Author

BPScott commented May 29, 2020

@evilebottnawi: Thanks! I've updated it to use typeof and have added some validate-options tests

@BPScott BPScott requested a review from alexander-akait May 29, 2020 20:30
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, please fix CI problems

@BPScott
Copy link
Contributor Author

BPScott commented Jun 1, 2020

CI looks unrelated. It's failing running webpack 5 tests on node 8 thanks to BigInt not existing. That seems reasonable as they bumped their minimum version a while back: webpack/webpack#9868

Sounds like we should remove wepback 5 in node 8 from the test matrix?

@alexander-akait
Copy link
Member

@BPScott Yes, let's test webpack@5 only for 10/12/14

@BPScott
Copy link
Contributor Author

BPScott commented Jun 3, 2020

#1089 Removes that CI check. I'll rebase this PR once #1089 is merged.

@alexander-akait
Copy link
Member

@BPScott merged 👍

@BPScott
Copy link
Contributor Author

BPScott commented Jun 3, 2020

rebased!

@alexander-akait alexander-akait merged commit 0902353 into webpack-contrib:master Jun 3, 2020
@alexander-akait
Copy link
Member

Thanks!

@marutypes
Copy link

Any idea when this will be released? I see that it hasn't been yet.

@alexander-akait
Copy link
Member

ETA is the next week

@marutypes
Copy link

Thanks!

@BPScott BPScott deleted the auto-function branch June 7, 2020 22:54
@marutypes
Copy link

@evilebottnawi is there any process we could help with to help get the updates released?

@alexander-akait
Copy link
Member

@TheMallen Make 48 hours in one day 😄 Release will be today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants