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.

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

Vite dev server optimize entries #18261

Closed
edikdeisling opened this issue Sep 27, 2021 · 7 comments
Closed

Vite dev server optimize entries #18261

edikdeisling opened this issue Sep 27, 2021 · 7 comments
Labels
CT Issue related to component testing OS: windows

Comments

@edikdeisling
Copy link
Contributor

edikdeisling commented Sep 27, 2021

Current behavior

I have a problems with component tests
If I add node_modules dependency into support file - this dependency wouldn't be pre-bundled by Vite. This is because Cypress pass supportFile into optimizeDeps.entries as is - and this is wrong for windows.

cypress/npm/vite-dev-server/src/startServer.ts
image

optimizeDeps.entries is array of fast-glob patterns

Desired behavior

All node_modules dependencies which are imported inside supportFile must be pre-bundled on windows too

Test code to reproduce

I've created repo to reproduce this
https://github.com/edikdeisling/test-cypress-cmp-tests-pre-bundling

Make sure there is no .vite in node_modules and run yarn test:component:run
image

Cypress Version

8.4.1

Other

There is another issue here to be honest. I think this is wrong to rewrite optimizeDeps.entries. Because of this behaviour I can't add my own optimizeDeps.entries
image

@lmiller1990
Copy link
Contributor

lmiller1990 commented Sep 28, 2021

We should be concat-ing with the optimizeDeps, not overwriting. Good catch, will look into both these.

@userquin
Copy link
Contributor

@edikdeisling @lmiller1990 I'm fixing this, just testing:

imagen

with result on @edikdeisling fork:

imagen

@userquin
Copy link
Contributor

userquin commented Sep 28, 2021

@lmiller1990 in a few minutes a PR on master branch as @JessicaSachs suggest me , please try to check and merge the unified-desktop-gui PR about fixing build on windows ASAP, since I need to patch each time I need to test some branch.

EDIT: just ignore the above and below comments about the solution on Windows, I don't know why my IntelliJ IDE didn't update the master branch correctly.

The main problem is when running on scripts with yarn rollup ...., just remove yarn from the script to allow on windows to resolve node to .cmd variant. On master branch and to be able to build the vite-dev-server package I need to modify these package.json files (their build scripts, ignore the startServer.ts, it has the changes on previous entry):

imagen

The other problem is on npm/create-cypress-tests package while copying cypress templates, this is not necessary to fix it since removing on local the copy part will just pass the build. I must change the build script with:

imagen

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 1, 2021

The code for this is done in cypress-io/cypress#18286, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot added stage: pending release and removed stage: needs review The PR code is done & tested, needs review labels Oct 1, 2021
@JessicaSachs
Copy link
Contributor

BTW we now use import { mergeConfig } from 'vite' to take in and merge user entires for optimizeDeps with our own. https://github.com/cypress-io/cypress/pull/20532/files#diff-42c5140b03bb18da2814e7ba54b69f632c2c28e5b425e0488e14cd386afd75daR9

@lmiller1990
Copy link
Contributor

So can we close this @JessicaSachs ?

@jennifer-shehane
Copy link
Member

Closing as addressed in #18286

@lmiller1990 lmiller1990 added CT Issue related to component testing and removed component testing labels Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CT Issue related to component testing OS: windows
Projects
None yet
Development

No branches or pull requests

8 participants
@chrisbreiding @jennifer-shehane @JessicaSachs @rockhold @userquin @lmiller1990 @edikdeisling and others