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

fix(optimizer): use correct default install state path for yarn PnP #19119

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

smeng9
Copy link
Contributor

@smeng9 smeng9 commented Jan 2, 2025

Fix #19118 for the yarn's default config. Main reason is if it can't find the file, it will use empty string to generate the hash, causing the result to be the same every time.

For yarn's custom config https://yarnpkg.com/configuration/yarnrc#installStatePath we probably need a different PR.

Also are we still going to support yarn 2.4 for optimize deps? Do we have a compatibility chart for vite version vs different package manger's version?
https://github.com/yarnpkg/berry/blob/93a56643ba3c813a87920dcf75c644eaf3b38e6f/CHANGELOG.md?plain=1#L392

bluwy
bluwy previously approved these changes Jan 2, 2025
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Good catch. It's also showing up as install-state.gz for me.

@bluwy
Copy link
Member

bluwy commented Jan 2, 2025

For yarn's custom config yarnpkg.com/configuration/yarnrc#installStatePath we probably need a different PR.

Maybe it's also possible for us to use a different file to detect? From https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored, we could also use .pnp.js or .pnp.cjs I think.

Also are we still going to support yarn 2.4 for optimize deps?

Ideally I think we should support all versions if it's not too hard to do so. Otherwise I think it's also fine for Vite to not support very old package managers. We don't really test against or track compatibility with different package managers.

@smeng9
Copy link
Contributor Author

smeng9 commented Jan 2, 2025

Hi @bluwy ,

Maybe it's also possible for us to use a different file to detect? From https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored, we could also use .pnp.js or .pnp.cjs I think.

I have applied your suggestion to check for .pnp.cjs/.pnp.js file so we don't need to deal with custom installStatePath

We don't really test against or track compatibility with different package managers.

It's not hard to add for .pnp.js for yarn v2 this time (https://yarnpkg.com/advanced/changelog#breaking-changes), but the order of files gets checked matters. .pnp.js should be checked after but not before checking the existence of .pnp.cjs https://yarnpkg.com/advanced/changelog#major-changes as the old file is not removed when updating yarn. Otherwise vite will never re-run the pre-bundle step if the unused file gets left on the disk after upgrading yarn from 2 directly to 4.

However I think probably in next major version release of vite we can specify supported package managers versions through package.json engines field for yarn https://endoflife.date/yarn and pnpm https://endoflife.date/pnpm so we no longer need to deal with edge cases like this.

@smeng9
Copy link
Contributor Author

smeng9 commented Jan 2, 2025

I guess the failing test is flaky, need to rerun.

@smeng9 smeng9 requested a review from bluwy January 2, 2025 20:23
@smeng9
Copy link
Contributor Author

smeng9 commented Jan 3, 2025

Switched back to calling yarn to get installStatePath, and if yarn is not installed will use default installStatePath location

@bluwy
Copy link
Member

bluwy commented Jan 3, 2025

Is there a reason to switch back to that. I don't think we can afford calling execSync when importing Vite, especially if the user don't use yarn either. I think the .pnp.cjs and .pnp.js was fine though

Otherwise vite will never re-run the pre-bundle step if the unused file gets left on the disk after upgrading yarn from 2 directly to 4.

I don't think we should worry about this. Shouldn't the user remove the .pnp.js themselves ultimately? Assuming it's pretty much a dormant file.

If all else doesn't work, I think we can fallback to yarn.lock still as that's the previous behaviour.

@smeng9
Copy link
Contributor Author

smeng9 commented Jan 3, 2025

@bluwy Reverted back to using .pnp.js/.pnp.cjs file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V6 regression: dependency optimizer pre-bundler did not re-run after lock file change
2 participants