-
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: force Webpack to emit assets on error #23844
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
npm/webpack-dev-server/__snapshots__/makeWebpackConfig.spec.ts.js
Outdated
Show resolved
Hide resolved
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.
Some suggestions, nice job!
Co-authored-by: Zachary Williams <ZachJW34@gmail.com>
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.
Overall looks good. Just need to update the version of the watiForSpecToFinish
command
Co-authored-by: Stokes Player <stokes@cypress.io>
}, | ||
devServerEvents: new EventEmitter(), | ||
} | ||
const actual = await makeWebpackConfig({ | ||
devServerConfig, | ||
sourceWebpackModulesResult: createModuleMatrixResult({ | ||
webpack: 4, | ||
webpack: 5, |
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.
This should be 5, not 4, since webpack-dev-server
v4 uses Webpack 5. Confused yet?
}, | ||
devServerEvents: new EventEmitter(), | ||
} | ||
const actual = await makeWebpackConfig({ | ||
devServerConfig, | ||
sourceWebpackModulesResult: sourceDefaultWebpackDependencies(devServerConfig), | ||
sourceWebpackModulesResult: createModuleMatrixResult({ |
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.
Updated this to use createModuleMatrixResult
, because we claim to be testing webpack-dev-server
v3 in this test, but the version of Webpack was getting resolved to v5, when it should be v4.
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!
User facing changelog
Compile errors will now be surfaced in the command log during testing for Angular and Next projects
Additional details
Some frameworks (Angular and Next) configure Webpack to not emit assets that have errored. So, if there's a compile error during development in these projects, instead of serving the module that has the compilation error and throwing it, Webpack serves the successful asset from the previous build. The result is that the compilation error only appears in the console, and our command log and AUT look fine - as if nothing has happened.
This behavior is undesirable because if there is a compilation error, we want to fail the test and make the user aware. This PR adds
emitOnErrors: true
(noEmitOnErrors: false
for Webpack 4) to our default Webpack configuration to override these values and force Webpack to emit modules even when there is an error regardless of the framework configuration.Steps to test
makeDefaultWebpackConfig.ts
angular.cy.ts
andnext.cy.ts
innpm/webpack-dev-server
makeDefaultWebpackConfig.ts
back and repeat step 2PR Tasks
cypress-documentation
?type definitions
?