fix(webpack-dev-server): do not encodeUri in loader #20575
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run 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 |
||||||||||||||||||||||||||
| import systemTests from '../lib/system-tests' | ||
|
|
||
| describe('@cypress/webpack-dev-server', function () { | ||
| systemTests.setup() |
There was a problem hiding this comment.
The start of the desperately needed system tests for WDS.
| @@ -18,7 +18,7 @@ const makeImport = (file: Cypress.Cypress['spec'], filename: string, chunkName: | |||
| const magicComments = chunkName ? `/* webpackChunkName: "${chunkName}" */` : '' | |||
|
|
|||
| return `"${filename}": { | |||
| shouldLoad: () => document.location.pathname.includes("${encodeURI(file.absolute)}"), | |||
There was a problem hiding this comment.
I guess we don't need this. If it passes CI, I'm pretty confident we are good to go. runner-ct as well as many projects in npm/react/examples exercise this code path.
There was a problem hiding this comment.
Add a test case for a spec with a file name with a space or unicode.
There was a problem hiding this comment.
We already had spaces: https://github.com/cypress-io/cypress/pull/20575/files#diff-f5c1d2cf3b3dd9c9d3c0464a216d71fdf03609a366cfe48406b699823ddce7f7R138
unit tests
...: https://github.com/cypress-io/cypress/pull/20575/files#diff-f5c1d2cf3b3dd9c9d3c0464a216d71fdf03609a366cfe48406b699823ddce7f7R119- unicode: https://github.com/cypress-io/cypress/pull/20575/files#diff-f5c1d2cf3b3dd9c9d3c0464a216d71fdf03609a366cfe48406b699823ddce7f7R100
system tests
- unicode https://github.com/cypress-io/cypress/pull/20575/files#diff-c1b1cdd1164c60f0f1508b4e9b16fc5e2adf5804ba4f90e1b95f8f4c7ba738f5
- spaces https://github.com/cypress-io/cypress/pull/20575/files#diff-d5beef32d6a21b22d7e5aa3a1763c6b60e07507b506b2e8ed9844485a494d8ca
...https://github.com/cypress-io/cypress/pull/20575/files#diff-296f863374bd7324f8407d91f8977f01f5266a3f4ffdc77b96ad895e5e9a9808
| @@ -18,7 +18,7 @@ const makeImport = (file: Cypress.Cypress['spec'], filename: string, chunkName: | |||
| const magicComments = chunkName ? `/* webpackChunkName: "${chunkName}" */` : '' | |||
|
|
|||
| return `"${filename}": { | |||
| shouldLoad: () => document.location.pathname.includes("${encodeURI(file.absolute)}"), | |||
There was a problem hiding this comment.
Add a test case for a spec with a file name with a space or unicode.
|
@lmiller1990 Does this close #5426 as well? |
|
@emilyrohrbough This is a CT specific change, so no this shouldn't fix the issue coming from e2e tests. |
There was a problem hiding this comment.
@lmiller1990 @JessicaSachs tested this with a filename with a space and confirmed it does not work. [] are valid in some URLs. We can change the shouldLoad to something like
// Uses the browser URL parsing method, so if it does change per browser we would still match
shouldLoad: () => document.location.pathname.includes("${new URL(`http://localhost${file.absolute}`).pathname}")
// alternative: see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI#encoding_for_ipv6
function fixedEncodeURI(str) {
return encodeURI(str).replace(/%5B/g, '[').replace(/%5D/g, ']');
}
shouldLoad: () => document.location.pathname.includes("${fixedEncodeURI(file.absolute)}"),
Test cases covering both brackets and encoded characters would be great.
|
Why wouldn't we just decodeURI at the other end of whatever is consuming this? |
|
@JessicaSachs We consume it in the same line, so we could do Haven't tested it but should work. |
|
@lmiller1990 created a PR targeting your branch that use |
| @@ -39,7 +39,7 @@ export const iframesController = { | |||
| // attach header data for webservers | |||
| // to properly intercept and serve assets from the correct src root | |||
| // TODO: define a contract for dev-server plugins to configure this behavior | |||
| req.headers.__cypress_spec_path = req.params[0] | |||
| req.headers.__cypress_spec_path = encodeURI(req.params[0]) | |||
There was a problem hiding this comment.
Needed this guy, too.
There was a problem hiding this comment.
Yes, when we request a spec from the AUT it goes through this controller on the way to webpack-dev-server.
|
@JessicaSachs @ZachJW34 This is ready to go, it has tests for all the suggested edge cases (unicode, spaces, dots, square brackets). We needed to add this line on the server: https://github.com/cypress-io/cypress/pull/20575/files#diff-f41297d10b0dfd5080e119a99dbb460a6ac6defccb4cf402b189c2e1e3b96947R42. I am not sure how this works with |
Agreed |
|
Two approvals - going to update with latest master, then merge. |
* Fix regression in path handling that had been previously fixed and lost in #20575 * Improve existing system test to cover this scenario
[or](Component Testing with Next.js) #20573User facing changelog
Correctly load specs with
[and]characters when using@cypress/webpack-dev-server.Additional details
Not really relevant to people outside Cypress, but when you back merge this to 10.0-release, you can use this branch for an example (same changes but updated system-tests project: https://github.com/cypress-io/cypress/compare/10.0-release...lmiller1990/20573?expand=1).
How has the user experience changed?
N/A
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?