chore: simplify glob usage #789
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #789 +/- ##
==========================================
+ Coverage 90.55% 91.48% +0.92%
==========================================
Files 2 2
Lines 413 411 -2
Branches 120 117 -3
==========================================
+ Hits 374 376 +2
+ Misses 38 34 -4
Partials 1 1 ☔ View full report in Codecov by Sentry. |
| @@ -407,12 +408,14 @@ class CopyPlugin { | |||
| logger.log(`begin globbing '${glob}'...`); | |||
|
|
|||
| /** | |||
| * @type {GlobEntry[]} | |||
| * @type {string[]} | |||
There was a problem hiding this comment.
whats the typedef change here?
There was a problem hiding this comment.
objectMode: true changes the return type from the default string[] to GlobEntry[]. Since we've removed objectMode: true we can go back to string[], which is simpler and has better performance
|
This code is quite well covered with tests: https://github.com/webpack-contrib/copy-webpack-plugin/blob/master/test/globOptions-option.test.js No test changes were necessary since the code is equivalent, but the tests did ensure that things work the same even when passing some pretty weird combinations of options. Let me know if there's something specific that you think should be covered with tests though |
src/index.js
Outdated
| */ | ||
| let globEntries; | ||
|
|
||
| try { | ||
| globEntries = await globby(glob, globOptions); | ||
| globEntries = globOptions.onlyDirectories |
There was a problem hiding this comment.
Let's avoid it and just add the onlyDirectories doesn't supported in documentation. It doesn't make sense for our plugin anyway.
This PR contains a:
Motivation / Use-Case
I started looking at implementing #788 and noticed that some of the glob usage here was more complicated than it needed to be.
The first thing that was being done with the glob results was to check if it's a file. However, globby has an
onlyFilesoption that we can use to avoid the need for that check. After that's done, there's no longer a need to setobjectMode. Thus, instead of getting back an object, we can simply receive back a string, which is both simpler and better for performance.Breaking Changes
n/a
Additional Info
I may follow up with another PR to switch to
tinyglobby. However, I thought it was better to separate those two refactorings to keep the PRs small and make it easier to debug if there are any unexpected issues