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

workflow: switch to pnpm #5060

Merged
merged 32 commits into from Sep 24, 2021
Merged

workflow: switch to pnpm #5060

merged 32 commits into from Sep 24, 2021

Conversation

yyx990803
Copy link
Member

No description provided.

@dominikg
Copy link
Contributor

scripts/release.js has a few uses of yarn.
here's how it was converted to pnpm in vite-plugin-svelte https://github.com/sveltejs/vite-plugin-svelte/blob/main/scripts/release.js

@dominikg
Copy link
Contributor

and on ci setup node action can also take care of pnpm cache now, https://github.com/sveltejs/vite-plugin-svelte/blob/36a087bd6b2cb303f13c6c91a3fae1decf26c5cc/.github/workflows/ci.yml#L31

@yyx990803
Copy link
Member Author

Ugh, looks like we have the tests passing on every platform but Windows... that's going to be very hard to debug...

@zkochan
Copy link

zkochan commented Sep 24, 2021

Let me know if you need some help or if there are some issues with pnpm that I need to prioritize.


- name: Build vite
run: yarn ci-build-vite
run: pnpm run ci-build-vite
Copy link

Choose a reason for hiding this comment

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

you may actually keep the Yarn syntax here if you wish. run is optional with pnpm as well.

Though you still have to escape options with -- like you did. We will probably make that work without -- in the future (pnpm/pnpm#3778)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm just doing it for consistency's sake

// pnpm treats file: protocols as link:, so it doesn't copy the actual files
// into node_modules, causing Vite to still consider those deps linked.
// This script is called from postinstall hooks in playground packages that
// uses the file: protocol, and copies the file: deps into node_modules.
Copy link

Choose a reason for hiding this comment

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

I think this issue is about a similar use case: pnpm/pnpm#3510

Copy link
Member Author

Choose a reason for hiding this comment

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

It does sound similar. I also tried using .npmrc inside a sub-package with package-import-method but it didn't seem to work.

@yyx990803
Copy link
Member Author

yyx990803 commented Sep 24, 2021

I believe this is now ready. But let's hold off until tomorrow's team meeting.

  • Tests are passing
  • Contribution guide updated
  • Relevant part in docs updated
  • Docs Netlify deploy config updated (also forces using locally built vite & plugin-vue via pnpm.override in package.json, build passing on Netlify)

Some notes on tests:

  • Due to pnpm's strictness, all playground repos must explicitly specify used deps (unless the dep is available in workspace root package.json)
  • Tests that rely on Yarn's file: protocol behavior must add a postinstall script that runs script/patchFileDeps to replicate Yarn's behavior.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

If you don't want to rush this, please give me the chance to test this on my local machine(s).
currently I'm on mobile, will have time in about next 12h

packages/playground/extensions/package.json Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
packages/playground/nested-deps/vite.config.js Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

Shinigami92 commented Sep 24, 2021

Following was made on Arch Linux x86_64 5.10.68-1-lts

Process for a newcomer/switcher to pnpm

  1. Checkout fork git clone git@github.<Username>/vite.git
  2. Install tools like yay or install nativly
    yarn is directly available via pacman (https://archlinux.org/packages/community/any/yarn/)
    pnpm needs to be installed via AUR (https://aur.archlinux.org/packages/pnpm-bin/)
  3. pnpm is not like yarn, you need to append install
  4. After executing pnpm install the pnpm-lock.yaml changed 👀
    I think we need to exclude pnpm-lock.yaml from prettier ⚠️ (.prettierignore line 9: yarn.lock -> pnpm-lock.yaml)

  • yarn build: 52s
  • pnpm build: 55s

  • yarn test-serve --runInBand: 1min 13s
  • pnpm test-serve -- --runInBand: 1min 18s

  • yarn test-build --runInBand: 1min 9s
  • pnpm test-build -- --runInBand: 1min 14s

@antfu
Copy link
Member

antfu commented Sep 24, 2021

Re: @Shinigami92

2: why not npm i -g pnpm or npx pnpm add -g pnpm? Many options here: https://pnpm.io/installation
3: actually that is more aligned to npm. Using either yarn or pnpm is already asking contributors to learn another tool, I didn't see much difference here.

Perf: benching build/test does not make much sense since most works are on the node side. Wondering what's the difference on deps installations?

Side note: yarn v3 failed to distinguish the binaries as well:
image

.github/workflows/ci.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Shinigami <chrissi92@hotmail.de>
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.

None yet

5 participants