Skip to content
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.

Sign up for GitHub

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: vite v3 not working with node >=17 #23048

Merged
merged 7 commits into from
Aug 2, 2022
Merged

fix: vite v3 not working with node >=17 #23048

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
de6d71b
fix: vite 3.0 not working with node >=17
ZachJW34 Aug 1, 2022
d985603
forgot to add system-test config
ZachJW34 Aug 1, 2022
bd0edb6
redundant map
ZachJW34 Aug 2, 2022
f9ffd78
Merge remote-tracking branch 'origin/develop' into zachw/issue-23042
ZachJW34 Aug 2, 2022
f321c30
make docker + npm happy
ZachJW34 Aug 2, 2022
e013251
make npm config cache dir
ZachJW34 Aug 2, 2022
1ead548
revert system test changes
ZachJW34 Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion npm/vite-dev-server/src/plugins/cypress.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export const Cypress = (
indexHtmlContent = indexHtmlContent.replace(
'<head>',
`<head>
${scriptTagsToInject.map((script) => script.toString())}
${scriptTagsToInject.map((script) => script.toString()).join('')}
Copy link
Contributor

@lmiller1990 lmiller1990 Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screen Shot 2022-08-02 at 12 13 04 PM

After:
Screen Shot 2022-08-02 at 12 12 25 PM

So this fix is more important that I thought.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do browsers do things like "move text from <head> into <body>???? 👎

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not seem to impact percy what-so-ever though! Weird.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the toString still necessary here since join should call toString on the contents?

Suggested change
${scriptTagsToInject.map((script) => script.toString()).join('')}
${scriptTagsToInject.join('')}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's clean I'll throw this in

`,
)

Expand Down
1 change: 1 addition & 0 deletions npm/vite-dev-server/src/resolveConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export const createViteDevServerConfig = async (config: ViteDevServerConfig, vit
cypressConfig.cypressBinaryRoot,
],
},
host: '127.0.0.1',
},
plugins: [
Cypress(config, vite),
Expand Down
9 changes: 9 additions & 0 deletions system-tests/projects/simple-ct/cypress-vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module.exports = {
component: {
supportFile: false,
devServer: {
bundler: 'vite',
viteConfig: {},
},
},
}
6 changes: 6 additions & 0 deletions system-tests/projects/simple-ct/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"dependencies": {
"vite": "^3.0.0",
"webpack": "^5.0.0"
}
}
Loading