Skip to content

feat(infra): adopt pnpm #7662

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

Merged
merged 6 commits into from
May 2, 2025
Merged

feat(infra): adopt pnpm #7662

merged 6 commits into from
May 2, 2025

Conversation

bmuenzenmeyer
Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer commented Apr 17, 2025

  • next.config.js, next-intl, i18n work — commit
  • pnpm build — import errors resolved, packages cleaned up
    • pnpm build when installed with --prod
  • pnpm dev
    • pnpm dev when installed with --prod
  • pnpm test - subpath import work needed
  • pnpm test:ci
  • pnpm lint
  • pnpm lint:fix
  • pnpm prettier
  • pnpm prettier:fix
  • pnpm format
  • pnpm storybook
  • pnpm storybook:build
  • pnpm check-types
  • pnpm scripts:release-post
  • Vercel Deployments
  • npm use cases

Tasks from Vote

Copy link

vercel bot commented Apr 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 2, 2025 8:43pm

@bmuenzenmeyer
Copy link
Collaborator Author

running full ci just for fun

@avivkeller
Copy link
Member

running full ci just for fun

FWIW because this changes the pull_request_target workflows, a full CI will almost certainly fail every time (as the main branch CI doesn't set up pnpm). What I would do is, add a workflow_dispatch trigger to each of the CIs you want to test, and trigger them with:

gh workflow run lint-and-tests.yml --ref pnpm

Before merging, just remove the trigger.

@avivkeller
Copy link
Member

Assuming that CI passes, all reviews, tasks, and failures have been addressed

@avivkeller
Copy link
Member

avivkeller commented May 1, 2025

Required checks, despite being listed as pending, pass on manual run:

Review dependencies fails because of GHSA-f7f6-9jq7-3rqj. AFAICT this isn't an issue for us, since all the content we are supplying has been manually reviewed (unlike a public forum, where the data is arbitrary)

@avivkeller
Copy link
Member

@nodejs/web-infra This cannot land via the merge queue. When you feel comfortable landing this (not now, but when your ready), you will need bypass the branch protection rules.

Co-authored-by: Matt Cowley <me@mattcowley.co.uk>
Signed-off-by: Aviv Keller <me@aviv.sh>
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

if we can ovoid pnpm and use node that would be nice

@avivkeller
Copy link
Member

avivkeller commented May 2, 2025

if we can ovoid pnpm and use node that would be nice

According to the vote, we are to use pnpm when possible.

#5334 (reply in thread)

@avivkeller
Copy link
Member

If there are no other objections, could someone with Bypass Rules permissions merge this on Monday? (I'm -1 on merging before the weekend, lest something bad happen)

@ovflowd ovflowd merged commit 2f9f1b2 into main May 2, 2025
12 of 13 checks passed
@ovflowd ovflowd deleted the pnpm branch May 2, 2025 22:28
@ovflowd
Copy link
Member

ovflowd commented May 2, 2025

If there are no other objections, could someone with Bypass Rules permissions merge this on Monday? (I'm -1 on merging before the weekend, lest something bad happen)

I'm feeling daring today :P

@ovflowd
Copy link
Member

ovflowd commented May 2, 2025

(Amazing work y'all, btw)

@flakey5
Copy link
Member

flakey5 commented May 2, 2025

(Amazing work y'all, btw)

+1

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.

8 participants