fix: vite v3 not working with node >=17 #23048
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Failures
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Binary tests I added aren't too happy |
|
Vite was working on Node.js 17 just fine, in my case. I wonder if it's either
|
| @@ -76,7 +76,7 @@ export const Cypress = ( | |||
| indexHtmlContent = indexHtmlContent.replace( | |||
| '<head>', | |||
| `<head> | |||
| ${scriptTagsToInject.map((script) => script.toString())} | |||
| ${scriptTagsToInject.map((script) => script.toString()).join('')} | |||
There was a problem hiding this comment.
Is this essential or just nice to have? Just curious
Right... "and a comma is removed from react + vite 3 apps in index.html". Argh lol... but since this is in <head> it shouldn't actually render anything, which is good - Percy would've picked this up.
There was a problem hiding this comment.
Check out the before video (near the end), it was actually rendering a comma that was visible. I don't think Percy would have caught this since we don't snapshot our system tests. Going to look into this a bit more, since like you said I shouldn't have seen the comma if it was in the <head> so somehow it's making it's way to the body
There was a problem hiding this comment.
After digging into this, I discovered that if we insert text into the head that doesn't parse correctly (in this case, we had the comma due to the stringified array), it will get shifted into the body.
Before:
After:
So this fix is more important that I thought.
There was a problem hiding this comment.
Why do browsers do things like "move text from <head> into <body>???? 👎
There was a problem hiding this comment.
Does not seem to impact percy what-so-ever though! Weird.
| @@ -76,7 +76,7 @@ export const Cypress = ( | |||
| indexHtmlContent = indexHtmlContent.replace( | |||
| '<head>', | |||
| `<head> | |||
| ${scriptTagsToInject.map((script) => script.toString())} | |||
| ${scriptTagsToInject.map((script) => script.toString()).join('')} | |||
There was a problem hiding this comment.
Is the toString still necessary here since join should call toString on the contents?
| ${scriptTagsToInject.map((script) => script.toString()).join('')} | |
| ${scriptTagsToInject.join('')} |
There was a problem hiding this comment.
That's clean I'll throw this in
User facing changelog
CT projects using Vite v3 work with Node versions >=17, and a comma is removed from react + vite 3 apps in index.html
Additional details
We now force the host to be
127.0.0.1when spinning up the vite-dev-server. We did the same fix for webpack a while ago (#21430) and Vite v3 needs a similar fix.I also fixed an issue that is only prevalent when using vite + react where a comma was being inserted into the served
index.htmldue to how we were injecting scripts.I added a vite setup for the
simple-ctsystem test proejct and added it to the binary tests to verify behavior across node versions.Steps to test
This issue can be seen inside our own codebase, just switch to a node version >= 17 and try running any our our vite component tests. See the original issue for repro steps, you can now point the locally running cypress at a vite 3 project using a node version >= 17 and it will now work.
How has the user experience changed?
Before: (Notice the comma on the second load when I switch back to Node 16)
Screen.Recording.2022-08-01.at.3.55.48.PM.mov
After:
Screen.Recording.2022-08-01.at.3.57.04.PM.mov
PR Tasks
cypress-documentation?type definitions?