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

copy getOptions from webpack #113

Merged
merged 3 commits into from Apr 13, 2021
Merged

copy getOptions from webpack #113

merged 3 commits into from Apr 13, 2021

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Apr 10, 2021

This PR contains a:

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

Motivation / Use-Case

Attempt to fix #106, a major compatibility problem with Webpack 5.

Breaking Changes

No.

Additional Info

Ideally we should be able to delegate computation to the main process, but unfortunately getOptions needs to return a value synchronously. In this PR I just copied the code verbatim. Copyright-wise, it should be sufficient to ask @alexander-akait and @sokra because they seem to be the only authors of the code.

@codecov
Copy link

codecov bot commented Apr 10, 2021

Codecov Report

Merging #113 (5b1945e) into master (78920d9) will decrease coverage by 0.81%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
- Coverage   21.84%   21.03%   -0.82%     
==========================================
  Files           8        8              
  Lines         467      485      +18     
  Branches       89       94       +5     
==========================================
  Hits          102      102              
- Misses        322      335      +13     
- Partials       43       48       +5     
Impacted Files Coverage Δ
src/worker.js 0.00% <0.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 78920d9...5b1945e. Read the comment docs.

@qnighy qnighy changed the title Get options copy getOptions from webpack Apr 10, 2021
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.

Ideally we should run webpack api here not copy this, because it can be bad for future webpack changes

@qnighy
Copy link
Contributor Author

qnighy commented Apr 11, 2021

Ideally we should run webpack api here not copy this, because it can be bad for future webpack changes

I agree, though as of 5.31.2 it's hard to call webpack's getOptions from workers because it's created in NormalModule.prototype.createLoaderContext.

Possible options:

  • Keep the status quo in this PR.
  • Expose getOptions implementation from webpack.
  • Port webpack's getOptions back to schema-utils or loader-utils or something, and use it from both webpack and thread-loader.

@qnighy
Copy link
Contributor Author

qnighy commented Apr 11, 2021

@alexander-akait here's another attempt: webpack/schema-utils#123. Which do you think is better?

@sokra
Copy link
Member

sokra commented Apr 13, 2021

Seems fine to copy the implementation. I don't think we will change that in webpack any time soon (it would be a breaking change anyway).

@alexander-akait alexander-akait merged commit d7531ef into webpack-contrib:master Apr 13, 2021
@qnighy qnighy deleted the getOptions branch April 14, 2021 09:37
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.

Webpack 5's "getOptions" is missing
3 participants