Skip to content

Conversation

@qnighy
Copy link

@qnighy qnighy commented Apr 11, 2021

This PR contains a:

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

Motivation / Use-Case

To help webpack/thread-loader#106.

webpack/thread-loader#113 will surely fix the issue above, but the PR will create multiple copies of getOptions implementation. webpack/thread-loader#113 (review)

This PR attempts to extract the common part from getOptions to reuse in webpack and thread-loader.

Breaking Changes

No.

Additional Info

This feature may not fit schema-utils' intended scope. I'm adding the feature here because it's a validation-related feature and webpack already depends on schema-utils, but I'm willing to do extra work if there's a better place for the feature.

cc: @alexander-akait

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #123 (5fe159a) into master (2e768a1) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   97.57%   97.63%   +0.06%     
==========================================
  Files           6        7       +1     
  Lines         700      720      +20     
  Branches      299      304       +5     
==========================================
+ Hits          683      703      +20     
  Misses         15       15              
  Partials        2        2              
Impacted Files Coverage Δ
src/getLoaderOptions.js 100.00% <100.00%> (ø)
src/index.js 100.00% <100.00%> (ø)

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 2e768a1...5fe159a. Read the comment docs.

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.

We should not put this logic here

@qnighy
Copy link
Author

qnighy commented Apr 12, 2021

@alexander-akait then where do you think is the best?

@alexander-akait
Copy link
Member

Inside thread-loader, we should run webpack API like we do with other loaders methods

@qnighy
Copy link
Author

qnighy commented Apr 12, 2021

Inside thread-loader, we should run webpack API like we do with other loaders methods

@alexander-akait That seems too difficult to me. Isn't there an option to release webpack/thread-loader#102 without getOptions for the time being? After that I might have a bandwidth for this.

@qnighy
Copy link
Author

qnighy commented Apr 14, 2021

Closing in favor of webpack/thread-loader#113. Thanks!

@qnighy qnighy closed this Apr 14, 2021
@qnighy qnighy deleted the getLoaderOptions branch April 14, 2021 09:36
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.

2 participants