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: shareable webpack configs using extends #3738

Merged
merged 20 commits into from
Apr 29, 2023

Conversation

burhanuday
Copy link
Member

@burhanuday burhanuday commented Apr 18, 2023

What kind of change does this PR introduce?

feature

Adds --extends option to the CLI as well as support for extends in webpack config

This PR builds on top of #2868
Instead of having to update webpack, this takes the approach of modifying the config before webpack is run

Did you add tests for your changes?
yes

If relevant, did you update the documentation?
no

Summary

Closes #2748

Does this PR introduce a breaking change?

Other information

@burhanuday
Copy link
Member Author

burhanuday commented Apr 18, 2023

Before continuing further work. A couple of questions

  1. What is the expected behaviour in case of multiple configs. If i use merge for extends, then everything gets merged into a single config which is not the expected behaviour. should we then limit extends to a single config and throw an error when multiple configs are passed?
  2. An extended config can also have its own extends property. In this case, do we want to recursively keep looking for extends or should we limit to some particular depth?

@alexander-akait @snitin315 Need your insights on this. Thank you

@alexander-akait
Copy link
Member

What is the expected behaviour in case of multiple configs. If i use merge for extends, then everything gets merged into a single config which is not the expected behaviour. should we then limit extends to a single config and throw an error when multiple configs are passed?

I think merge it in each other, you can have webpack.config.base.js and have multiple configuration which extend base, for example server and client

An extended config can also have its own extends property. In this case, do we want to recursively keep looking for extends or should we limit to some particular depth?

We can prevent it to keepind array of merged and if it developer has a loop throw an error

@snitin315
Copy link
Member

An extended config can also have its own extends property. In this case, do we want to recursively keep looking for extends or should we limit to some particular depth?

We can prevent it to keepind array of merged and if it developer has a loop throw an error

I added a few tests cases in #2868. I feel those should be covered.

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #3738 (ce7c6cb) into master (f052d3f) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3738      +/-   ##
==========================================
+ Coverage   91.46%   91.64%   +0.17%     
==========================================
  Files          22       22              
  Lines        1581     1627      +46     
  Branches      444      460      +16     
==========================================
+ Hits         1446     1491      +45     
- Misses        135      136       +1     
Impacted Files Coverage Δ
packages/configtest/src/index.ts 96.42% <100.00%> (+0.42%) ⬆️
packages/webpack-cli/src/plugins/cli-plugin.ts 100.00% <100.00%> (ø)
packages/webpack-cli/src/webpack-cli.ts 93.04% <100.00%> (+0.18%) ⬆️

Continue to review full report in Codecov by Sentry.

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

@burhanuday
Copy link
Member Author

An extended config can also have its own extends property. In this case, do we want to recursively keep looking for extends or should we limit to some particular depth?

We can prevent it to keepind array of merged and if it developer has a loop throw an error

I added a few tests cases in #2868. I feel those should be covered.

Got it. Most of the implementation is done, just need to handle the part where an extended config has its own extends property


config.options[index] = merge(extendedConfig, configOptions);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please move it to own function and just use && { extend: string[] } to avoid ts-except-error

@burhanuday burhanuday marked this pull request as ready for review April 25, 2023 04:23
@burhanuday burhanuday requested a review from a team as a code owner April 25, 2023 04:23
@burhanuday burhanuday changed the title [Draft] feat: shareable webpack configs using extends feat: shareable webpack configs using extends Apr 25, 2023
@burhanuday
Copy link
Member Author

All checks have passed

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.

Thank you, looks good, I will test it soon (maybe add a little bit more tests) and we will merge, thank you

@alexander-akait
Copy link
Member

alexander-akait commented Apr 29, 2023

While refactoring found bugs...

It should work now as expected, also allows to load configurations on any depth + catch recusive calls + setup valid cache keys for invalidation, small tweaks.

But yeah we need more tests here:

  • deep extends
  • multiple extends in webpack.config.js
  • multiple --extends
  • recusive calls
  • mix extends in webpack.config.js and --extends (also check buildDependencies to undestand we have all loaded configuration paths)
  • test with --extends and --merge (also check buildDependencies to undestand we have all loaded configuration paths)
  • test multicompiler mode with extends

In theory we can add even more tests, but I think it is enough for basic testing

@alexander-akait
Copy link
Member

So if somebody have time feel free to add such tests cases

@snitin315
Copy link
Member

Do we add them in the same PR or can we add them separately?

@alexander-akait
Copy link
Member

We need to ad them here

@snitin315
Copy link
Member

@burhanuday Would you be willing to send a PR to add this feature to the CLI docs here?

@burhanuday
Copy link
Member Author

@snitin315 yup. Will do

@burhanuday
Copy link
Member Author

@snitin315 @alexander-akait can we do a release for webpack-cli before webpack release with the type changes get published tomorrow

@alexander-akait
Copy link
Member

@burhanuday better to do it after webpack release, otherwise we will have types problems, release will be tomorrow, so I will do webpack-cli release tomorrow too

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

Successfully merging this pull request may close these issues.

Shareable Configs
4 participants