feat: update to Vite 3 #22915

lmiller1990 · 2022-07-25T11:36:01Z

Closes Vite 3 not working with Cypress Component Testing #22784

User facing changelog

Update Cypress monorepo to use Vite 3
As part of the Vite 3 update, I fixed a bug in our vite-dev-server integration that was introduced by a breaking change in Vite 3. The bug was Vite plugins that modify index.html were not correctly inserted into the index.html (eg @vitejs/plugin-react, example issue.

Additional details

Vite 3 came out, it had some breaking changes. Breaking changes. [#6901] fix: sequential injection of tags in transformIndexHtml broke us, specifically.

It's fixed in this PR, see here.

Note: Percy has 51 changes - I actually fixed a bug, Percy was capturing the UI in the incorrect state. See this comment on the problem. I will hit approve before we merge, once everyone is happy the PR.

Steps to test

CI should be passing, and since we use Vite internally a lot, if it's passing, everything should be 💯.

I'd also recommend pulling down the repo, and doing a yarn dev in something like packages/app with component testing, and making sure it all works as expected.

If you like, you could try replicate this user issue and make it it works: vitejs/vite#9362 (we have tests for this specific case - using React and the Vite plugin - but good to be certain).

How has the user experience changed?

Can use Vite 3 with Cypress Component Testing.

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?

cypress-bot · 2022-07-25T11:36:05Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-07-25T12:09:43Z

Test summary

4381 • 0 • 49 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`84ee67e`
Started	Jul 27, 2022 12:10 AM
Ended	Jul 27, 2022 12:22 AM
Duration	11:53 💡
OS	Linux Debian - 11.3
Browser	Electron 102

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

lmiller1990 · 2022-07-26T00:19:58Z

npm/react/package.json

@@ -21,7 +21,6 @@
    "@vitejs/plugin-react": "1.3.1",
    "axios": "0.21.2",
    "cypress": "0.0.0-development",
-    "cypress-plugin-snapshots": "1.4.4",


This was actually messing things up by introducing a transitive dependency. I wrote about it here and ultimately did the change as part of this PR.

lmiller1990 · 2022-07-26T00:29:07Z

npm/vite-dev-server/src/plugins/cypress.ts

+      // receive and inject it.
+      // For now we just grab any `<script>` tags and inject them to <head>.
+      // We will add more handling as we learn the use cases.
+      const root = parse(html)


So, what this is doing is basically grabbing any <script> tags from the html that might have been inserted by other plugins and sticking them inside our HTML. An example is the vitejs/react-plugin, which injects this code: https://github.com/vitejs/vite/blob/main/packages/plugin-react/src/fast-refresh.ts#L32-L36. A user already reported this: vitejs/vite#9362.

Looking at the Vite docs for this function and it makes me wonder if we could skip the logic of re-inserting the scripts and let Vite do it for us by returning our file and an array of tags we want to propagate

(emphasis mine)

The hook can be async and can return one of the following:

Transformed HTML string

An array of tag descriptor objects ({ tag, attrs, children }) to inject to the existing HTML. Each tag can also specify where it should be injected to (default is prepending to )

An object containing both as { html, tags }

Had a similar instinct, but I ran into a few roadblocks with this. With regards to using the { html, tags } API for the HTML, by default we inject something like this:

<!DOCTYPE html> <html> <head> <meta charset="utf-8"> <meta http-equiv="X-UA-Compatible" content="IE=edge"> <meta name="viewport" content="width=device-width,initial-scale=1.0"> <title>Components App</title> </head> <body> <div data-cy-root></div> </body> </html>

Let's call this baseHtml. Users can customize this. We could do:

return { html: baseHtml, tags: /* tags to inject */ }

Injecting the script content is pretty awkward, though, since we need to parse the attrs to use the tags API, eg:

const tags = scriptTagsToInject.map<HtmlTagDescriptor>(script => ({ tag: 'script', injectTo: 'head', attrs: { // Is it src="..." or module="..."? // We gotta pass every possible attribute using`node-html-parser`, which is pretty complex. } })) return { html: newHtml, tags }

So we need to parse the tag in even more detail. For many plugins, this API works great, like plugin-react, since they know they want to inject type="module": see here. For us to use this API, we need to completely pass everything, which can include <script>, <div>, etc - this seems challenging and prone to error.

I could be missing something, it would be much nicer to use the Vite API than string substitution, definitely open to improving this if you see some alternatives. Right now there are definitely edge case, for example, if another plugin injects a <div> or something in <head> we won't grab it.

Yuck. Makes sense to just stick with the string sub for now, adding that level of additional parsing feels very brittle and error-prone

lmiller1990 · 2022-07-26T00:29:35Z

npm/vite-dev-server/src/plugins/cypress.ts

+
+      const scriptTagsToInject = root.childNodes.filter((node) => {
+        // @ts-ignore
+        return node.rawTagName === 'script'


Also, the type defs in node-html-parser seem to be wrong.

I think this is because childNodes is an array of Node which is an abstract class - implementations include TextNode for comments (which have no tag name) and HTMLElement which has the rawTagName field. To make TS happy you'd need to do a type-check here then you could use the field name without the linter override

Nice catch! I just did:

const scriptTagsToInject = root.childNodes.filter((node) => { return node instanceof HTMLElement && node.rawTagName === 'script' })

Here: 0d12d60

lmiller1990 · 2022-07-26T00:33:57Z

packages/app/vite.config.mjs

@@ -1,4 +1,4 @@
-import { makeConfig } from '../frontend-shared/vite.config'
+import { makeConfig } from '../frontend-shared/vite.config.mjs'


So Vite is all-in on ESM it seems, we cannot use ts like this. We either need type: module, or something r;dr... I'm not sure what. I tried a bunch of things but no luck.

I think vite.config.mts should also work, except we need to update TypeScript, which is pretty involved. The least invasive change is to use mjs here. Once we update to TypeScript 4.7, .mts should work.

I am a little bit tired of this module madness. I thought we were finally coming to an agreement in the ecosystem, but now we have mts, too.

Worth writing up an issue and tagging it here for eventual cleanup? I suspect we'll never circle back unless we're reminded

Good idea, done: #22945.

I think it's probably a config and/or TS version related issus.

lmiller1990 · 2022-07-26T00:39:00Z

packages/frontend-shared/src/components/ShikiHighlight.vue

@@ -72,51 +72,12 @@ shikiWrapperClasses computed property.
  </div>
 </template>

-<script lang="ts">


Moved to it's own file. Vite is more strict and complained about this export.

In general I think it's better if vue files only have 1 default export, anyway.

lmiller1990 · 2022-07-26T00:40:31Z

packages/frontend-shared/src/styles/shared.scss

@@ -10,17 +10,6 @@
    local("Roboto-Light"), local("DroidSans"), local("Tahoma");
 }

-/* Set up Roboto for modern browsers, all weights */


I think this is actually incorrect, and introduced inconsistencies in the style of our code font.

I deleted it. Of course, now Percy is showing lots of changes - the snapshots are now more consistent.

Note when looking below, focus on the g in integration (the word integration appears twice).

Before (Percy)

After (Percy)

Figma (how it should be)

You can see that, based on the Figma mocks, the font should be the one with the stylized, curvy g.

lmiller1990 · 2022-07-26T00:42:32Z

system-tests/projects/vite3.0.2-react/package.json

@@ -0,0 +1,8 @@
+{


Above file has the Vite 3 test:

https://github.com/cypress-io/cypress/pull/22915/files#diff-e6bb614dd5b32e07eb125231dc627f64eb0e9a36b6c39cc7b6d23e802b9404b1

mike-plummer

Pulled down locally and everything seems to be working as expected. Left a couple "whatabout" comments that I'll leave up to you whether to incorporate, and one for a TS issue that might be good to fix

mike-plummer · 2022-07-26T13:13:15Z

npm/react/package.json

@@ -30,7 +29,7 @@
    "rollup": "^2.38.5",
    "rollup-plugin-typescript2": "^0.29.0",
    "typescript": "^4.2.3",
-    "vite": "2.9.5",
+    "vite": "3.0.2",


Just an FYI - Vite released v3.0.3 yesterday (CHANGELOG)

I updated to Vite 3.0.3! 84ee67e

mike-plummer · 2022-07-26T13:29:43Z

npm/vite-dev-server/src/plugins/cypress.ts

+      // receive and inject it.
+      // For now we just grab any `<script>` tags and inject them to <head>.
+      // We will add more handling as we learn the use cases.
+      const root = parse(html)


Looking at the Vite docs for this function and it makes me wonder if we could skip the logic of re-inserting the scripts and let Vite do it for us by returning our file and an array of tags we want to propagate

(emphasis mine)

The hook can be async and can return one of the following:

Transformed HTML string

An array of tag descriptor objects ({ tag, attrs, children }) to inject to the existing HTML. Each tag can also specify where it should be injected to (default is prepending to )

An object containing both as { html, tags }

mike-plummer · 2022-07-26T13:56:25Z

packages/app/vite.config.mjs

@@ -1,4 +1,4 @@
-import { makeConfig } from '../frontend-shared/vite.config'
+import { makeConfig } from '../frontend-shared/vite.config.mjs'


Worth writing up an issue and tagging it here for eventual cleanup? I suspect we'll never circle back unless we're reminded

mike-plummer · 2022-07-26T13:58:38Z

npm/vite-dev-server/src/plugins/cypress.ts

+      let indexHtmlContent = await fs.promises.readFile(indexHtmlPath, { encoding: 'utf8' })
+
+      // Inject the script tags
+      indexHtmlContent = indexHtmlContent.replace(
+        '<head>',
+        `<head>
+        ${scriptTagsToInject.map((script) => script.toString())}
+      `,
+      )


Total nitpick, but could avoid the let here by promisifying this:

const indexHtmlContent = await fs.promises.readFile(indexHtmlPath, { encoding: 'utf8' }) .then((content) => { // Inject the script tags return content.replace( '<head>', `<head> ${scriptTagsToInject.map((script) => script.toString())} `, ) })

Both fine, I think I like let + reassign a little more than then, but good to know this is an option, too.

Thoughts on mixing await and then? I usually pick one or the other, but maybe combining them can make code more readable (and avoid mutation).

I agree that it's usually best to avoid mixing async/await and then paradigms. I guess there is one other option to just wrap the await statement with parens but that isn't super-elegant either, so feel free to keep it the way it is:

const indexHtmlContent = (await fs.promises.readFile(indexHtmlPath, { encoding: 'utf8' })) // Inject the script tags .replace( '<head>', `<head> ${scriptTagsToInject.map((script) => script.toString())} `, )

mike-plummer · 2022-07-26T14:03:21Z

npm/vite-dev-server/src/plugins/cypress.ts

+
+      const scriptTagsToInject = root.childNodes.filter((node) => {
+        // @ts-ignore
+        return node.rawTagName === 'script'


I think this is because childNodes is an array of Node which is an abstract class - implementations include TextNode for comments (which have no tag name) and HTMLElement which has the rawTagName field. To make TS happy you'd need to do a type-check here then you could use the field name without the linter override

ZachJW34

Tested it out and it works great! LGTM!!

This reverts commit 6adba46.

lmiller1990 added 3 commits July 25, 2022 17:39

chore: do not rely on transitive dep. from npm/react

0c50950

chore: bump to vite 3.0.2

bf83f33

revert

8e04971

lmiller1990 mentioned this pull request Jul 25, 2022

chore: Update to Vite 3, TS 4.7.4 #22875

Closed

4 tasks

target es6

5f2e42f

This was referenced Jul 25, 2022

chore: do not rely on transitive dep. from npm/react #22914

Closed

Cypress component test produces "@vitejs/plugin-react can't detect preamble. Something is wrong" vitejs/vite#9362

Closed

Vite 3 not working with Cypress Component Testing #22784

Closed

remove unused code

cbc65fe

lmiller1990 marked this pull request as ready for review July 26, 2022 00:18

lmiller1990 requested review from a team as code owners July 26, 2022 00:18

lmiller1990 requested review from AtofStryker and removed request for a team and AtofStryker July 26, 2022 00:18

lmiller1990 commented Jul 26, 2022

View reviewed changes

update styles

fc9c4e1

lmiller1990 changed the title ~~chore: update to Vite 3~~ feat: update to Vite 3 Jul 26, 2022

mike-plummer approved these changes Jul 26, 2022

View reviewed changes

mike-plummer requested a review from a team July 26, 2022 14:04

lmiller1990 added 2 commits July 27, 2022 09:26

merge in develop

bc5f085

remove ts-ignore

0d12d60

bump to vite 3.0.3

84ee67e

lmiller1990 mentioned this pull request Jul 26, 2022

vite.config.ts not working with Vite 3 (internal monorepo) #22945

Open

ZachJW34 approved these changes Jul 27, 2022

View reviewed changes

lmiller1990 merged commit 6adba46 into develop Jul 28, 2022

lmiller1990 deleted the lmiller/vite-bump-v2 branch July 28, 2022 00:00

tbiethman added a commit that referenced this pull request Aug 1, 2022

Revert "feat: update to Vite 3 (#22915)"

05a3bf1

This reverts commit 6adba46.

lmiller1990 mentioned this pull request Aug 2, 2022

fix: windows CI with Vite 3 #23052

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update to Vite 3 #22915

feat: update to Vite 3 #22915

lmiller1990 commented Jul 25, 2022 •

edited

Loading

cypress-bot bot commented Jul 25, 2022

cypress bot commented Jul 25, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022

lmiller1990 Jul 26, 2022

mike-plummer Jul 26, 2022

lmiller1990 Jul 26, 2022 •

edited

Loading

mike-plummer Jul 27, 2022

lmiller1990 Jul 26, 2022

mike-plummer Jul 26, 2022

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022

lmiller1990 Jul 26, 2022 •

edited

Loading

mike-plummer Jul 26, 2022

lmiller1990 Jul 26, 2022

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022

mike-plummer left a comment

mike-plummer Jul 26, 2022

lmiller1990 Jul 26, 2022

mike-plummer Jul 26, 2022

mike-plummer Jul 26, 2022

mike-plummer Jul 26, 2022

lmiller1990 Jul 26, 2022

mike-plummer Jul 27, 2022

mike-plummer Jul 26, 2022

ZachJW34 left a comment

		@@ -1,4 +1,4 @@
		import { makeConfig } from '../frontend-shared/vite.config'
		import { makeConfig } from '../frontend-shared/vite.config.mjs'

feat: update to Vite 3 #22915

feat: update to Vite 3 #22915

Conversation

lmiller1990 commented Jul 25, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jul 25, 2022

cypress bot commented Jul 25, 2022 • edited Loading

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

lmiller1990 Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Before (Percy)

After (Percy)

Figma (how it should be)

Choose a reason for hiding this comment

mike-plummer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

lmiller1990 commented Jul 25, 2022 •

edited

Loading

cypress bot commented Jul 25, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading

lmiller1990 Jul 26, 2022 •

edited

Loading