fix: Ensuring spec file presence prior to webpack-dev-server compilation #21550
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run 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 |
| @@ -8,4 +8,39 @@ describe('Config options', () => { | |||
| cy.contains('App.cy.jsx').click() | |||
| cy.get('.passed > .num').should('contain', 1) | |||
| }) | |||
|
|
|||
| // https://cypress-io.atlassian.net/browse/UNIFY-1697 | |||
| it('filters missing spec files from loader during pre-compilation', () => { | |||
There was a problem hiding this comment.
Awesome test, the cy-in-cy tests for the dev-servers are so darn cool.
There was a problem hiding this comment.
They are quite cool, but I wonder if a regression test would be best added in the actual npm/webpack-dev-server @tbiethman? This test is quite far away from the actual code it is related to. Not sure how practical it is to add unit/integration test inside of webpack-dev-server, though.
|
|
||
| // Ensure we don't try to load files that have been removed from the file system | ||
| // but have not yet been detected by the onSpecsChange handler | ||
| this.files = (this.files || []).filter((file) => fs.existsSync(file.absolute)) |
There was a problem hiding this comment.
I am not sure we want to use existsSync here, should we use the async pathExists from fs-extra and tapAsync?
There was a problem hiding this comment.
https://webpack.js.org/api/compiler-hooks/#beforecompile can work with tapAsync
There was a problem hiding this comment.
Or - is the point we must do this async, so we avoid the race condition?
There was a problem hiding this comment.
We just need this to execute prior to the compile phase, which should happen regardless of whether we're sync/async here. Given that we're not actively manipulating the compile params here, we might as well use async and get whatever benefit that might have.
Your example branch was helpful, thank you. I thought about going with a Promise.all rather than running them in series. Something like:
const foundFiles = await Promise.all(this.files.map(async (file) => {
try {
const exists = await fs.pathExists(file.absolute)
return exists ? file : null
} catch (e) {
return null
}
}))
this.files = foundFiles.filter((file) => file !== null)Thoughts?
|
I updated this to use an async beforeCompile implementation, seems to work great. I also added a mocha integration test to get slightly more granular coverage of this behavior. To help with this, I added a new event ( |
There was a problem hiding this comment.
Tests were improved, looks good. @ZachJW34 had previously QA'd so I'll merge this up.
User facing changelog
Additional details
There is a race condition between our webpack compilation and our emission of the
dev-server:specs:changedevent. When a spec file is removed from the file system, we attempt to compile prior to updating our internalfilesproperty within the plugin, causing the loader to reference files that no longer exist.I updated the
CypressCTWebpackPluginto filter out files in state that no longer exist on the file system during the beforeCompile phase. This prevents both the initial error from being presented at file removal time and allows the file to be immediately executed when re-created.How has the user experience changed?
webpack_delete_spec.mov
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?