-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: allow vite/webpack config to be an async function #23605
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
let resolvedOverrides: UserConfig = {} | ||
|
||
if (typeof viteOverrides === 'function') { | ||
resolvedOverrides = await viteOverrides(viteBaseConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to utilize the loadConfigFromFile function before executing the viteConfig function? That way, the config passed to the function would have the resolved values from the configFile
that they could modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool idea, but the loadConfigFromFile
function is new in Vite 3; since we support Vite 2, we won't be able to use it if we want to have the same experience across both versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! One thing I would like to see is some unit tests in npm/webpack-dev-server/test/makeWebpackConfig.spec.ts
and npm/vite-dev-server/test/resolveConfig.spec.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one tiny question/nit
| ((baseConfig: Configuration) => Promise<Configuration>) | ||
| ((baseConfig: Configuration) => Configuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on consolidating the two function types and just vary the return type?
(baseConfig: Configuration) => (Configuration | Promise<Configuration>)
Side note: Should the return types be Promise<Partial<Configuration>>
and Partial<Configuration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, this is obviously better than what I had - I don't know why I wrote it like this 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need Partial
at all - every property in Configuration
is nullable already (via webpack's type defs) but I'll add it for good measure, since we seem to do it in a few other places.
Thanks for the reviews! @mike-plummer @ZachJW34 updates with tests and improved types: 723e306 Also I found some edges cases and decided we should slightly simplifying this. Beforecomponent: {
devServer: {
webpackConfig: async (baseConfig) => {
return /* whatever you want */
}
}
} Aftercomponent: {
devServer: {
webpackConfig: async () => {
return /* whatever you want */
}
}
} The change is we do not pass a baseConfig to the callback. It's not even clear what it should be - the CRA/Next.js config (if we found one? Should we pass that?) We probably do NOT want to pass in our internal baseConfig, that'll give the users opportunities to remove our plugins and break things by accident. Also there is many edge cases we'd need to consider we do try to pass in their config we found. Vite has a
Which is what we already do. This should still solve the problems we wanted to with this API - support for asynchronously modifying the config - as well as avoid needing to do the whole Node.js module dance, which is a mess (not just here, but in the entire ecosystem - even Vite has some problems with certain module combinations when you throw TS into the mix). I updated the docs: cypress-io/cypress-documentation#4699 to reflect this. I'll run it by Brian next meeting but I think this should be fine - if anything, we should do the minimum changes to support the ecosystem and community, we can revisit passing a baseConfig to the callback if it's needed. |
Merging is blocked until this is reviewed and finalized by product (that would be Brian). |
User facing changelog
Add support for advanced dev server configuration via an async function that can optionally modify the dev server config.
Additional details
Mainly useful for third party tooling that wants to ship a built in or pre-configured solution for Component Testing using Cypress. Case study and info here: #22390 (comment)
Steps to test
Not much to do - just look at the test I added and see that they are passing.
Please review the docs PR, too: cypress-io/cypress-documentation#4699
How has the user experience changed?
No change.
PR Tasks
cypress-documentation
?type definitions
?