-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix: map framework name to dependency that contains webpack #22774
Conversation
Thanks for taking the time to open a PR!
|
Nice catch. Any potential edge case/complexity when Nuxt 3 comes with webpack + vite? Just add an additional conditional? |
@@ -56,7 +56,18 @@ export const cypressWebpackPath = (config: WebpackDevServerConfig) => { | |||
}) | |||
} | |||
|
|||
// Source the users framework from the provided projectRoot. The framework, if available, will server | |||
type FrameworkWebpackMapper = { [Property in typeof ALL_FRAMEWORKS[number]]: string | undefined } |
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.
We could define this as type FrameworkWebpackMapper = { [Property in typeof ALL_FRAMEWORKS[number]]?: string }
so the undefined
entries aren't necessary for react & vue.
That said, I suppose it might be desired to have a dev actually look and think about this logic when we add new frameworks, so having the TS compiler complain if they don't explicitly add an entry for it might be good?
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.
That's what I was going for it. Whenever a new framework is added, this will enforce that whoever is adding the new framework considers how to properly link the framework name to the package that contains the relevant deps.
@lmiller1990 I set up a Nuxt3 app with
to use Webpack. This won't work out of the box, as it requires you to install |
While working through the snapshots, I realized our system tests weren't testing what we wanted them to. All of the dependencies (webpack, dev-server etc..) were being sourced from the root of the repo rather than the tmp project. This PR corrects that behavior, here is a before and after of the sources
I also tweaked the webpack stdout replacers to remove dynamic data, such as port numbers and timings |
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 |
@ZachJW34 good catch on the webpack + system tests problem. I had the EXACT same thing in the React 18 mega PR, it was very frustrating, I'm glad we are improving this. ✅ Agreed, let's deal with Nuxt separately, but it is coming soon, so we should figure out what to do. |
User facing changelog
na
Additional details
When sourcing Webpack dependencies for
@cypress/webpack-dev-server
, we use the provided framework as therequire.resolve
searchRoot if provided. We need to map the provided framework to the corresponding dependency for this to make sense i.e.create-react-app
->react-scripts
. This was working due to the recursive nature ofrequire.resolve
and our fallback to searchprojectRoot
is we can't find the framework, but we want to start the search at the package that might contain webpack in case there are hoisting conflicts.Steps to test
All system tests for
@cypress/webpack-dev-server
are green, and I've added some unit tests to the handlers to confirm the new behavior.How has the user experience changed?
na, this should fix any bugs some users might have had if
webpack
wasn't hoisted to the root of their project'snode_modules
PR Tasks
cypress-documentation
?type definitions
?