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(webpack-plugin): webpack 5 configuration factory #2776

Merged
merged 12 commits into from
Jun 16, 2022

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Mar 23, 2022

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The test suite passes successfully on my local machine (if applicable).

Summarize your changes:

Another take at #1556 for Webpack 5

@erickzhao erickzhao changed the title Erick/webpack 5 configurationfactory webpack 5 configurationfactory Mar 23, 2022
@erickzhao erickzhao changed the title webpack 5 configurationfactory feat(webpack-plugin): webpack 5 configurationfactory Mar 23, 2022
@erickzhao erickzhao changed the title feat(webpack-plugin): webpack 5 configurationfactory feat(webpack-plugin): webpack 5 configuration factory Mar 24, 2022
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #2776 (a3613e6) into master (9026eae) will increase coverage by 0.09%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #2776      +/-   ##
==========================================
+ Coverage   73.93%   74.03%   +0.09%     
==========================================
  Files          77       78       +1     
  Lines        2356     2365       +9     
  Branches      436      437       +1     
==========================================
+ Hits         1742     1751       +9     
  Misses        464      464              
  Partials      150      150              
Impacted Files Coverage Δ
packages/plugin/webpack/src/WebpackPlugin.ts 37.94% <0.00%> (ø)
packages/plugin/webpack/src/WebpackConfig.ts 98.76% <100.00%> (+0.03%) ⬆️
packages/plugin/webpack/src/util/processConfig.ts 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 9026eae...a3613e6. Read the comment docs.

@erickzhao erickzhao marked this pull request as ready for review April 19, 2022 17:40
const rawConfig =
typeof config === 'string'
? // eslint-disable-next-line import/no-dynamic-require, global-require, @typescript-eslint/no-var-requires
(require(path.resolve(this.projectDir, config)) as Configuration | ConfigurationFactory)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this were ESM compatible, so that the config file here could be an ESM module (potentially w/ top-level await). I think @MarshallOfSound mentioned a module that will do the work of figuring out whether a file is CommonJS or ESM and "just do the right thing" i.e. require() or import()? Or maybe just import() is enough here?

Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Tested this out locally, and it seems to be working well. I agree with nornagon above that the ESM compatibility would be nice to have, but if we want to merge this now and start working with it, we can make a tracking issue for that feature 👍

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.

5 participants