-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore: updated commands, use plain node #7850
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Lighthouse Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces various pnpm
commands with node --run
or npx
invocations, reorders some JSON arrays for consistency, and updates engine definitions to use devEngines
.
- Switches all package scripts and GitHub workflows from
pnpm
tonode --run
ornpx
- Alphabetizes file lists in
package.json
and repositions license/type fields - Introduces
devEngines
and removes customlint:lint-staged
&check-types
commands
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
turbo.json | Removed obsolete tasks and renamed check-types to lint:types |
pnpm-workspace.yaml | Added new build dependencies under onlyBuiltDependencies |
packages/ui-components/package.json | Sorted files , swapped check-types for lint:types and updated scripts |
packages/ui-components/eslint.config.js | Updated import for flatConfigs |
packages/i18n/package.json | Alphabetized files array |
packages/i18n/eslint.config.js | Destructured flatConfigs import |
package.json | Added type: module , license , devEngines , and switched scripts to node --run |
apps/site/package.json | Converted all pnpm scripts to node --run and added Cloudflare commands |
apps/site/eslint.config.js | Updated importX usage |
CONTRIBUTING.md | Updated usage examples from pnpm to node --run |
.lintstagedrc.json | Swapped turbo run lint:lint-staged for node --run lint |
.husky/pre-commit | Replaced pnpm hooks with node --run lint:staged and lint:types |
.github/workflows/*.yml | Replaced pnpm commands with npx or node --run |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
pnpm-workspace.yaml:10
- [nitpick] The
onlyBuiltDependencies
list is not alphabetized, which can make future updates harder to manage. Consider sorting these entries alphabetically for consistency.
- - '@tailwindcss/oxide'
CONTRIBUTING.md:155
- [nitpick] Markdown list items use inconsistent bullet markers (
--
vs-
). Standardize to a single-
prefix for readability and to adhere to common Markdown style.
-- `pnpm dev` runs Next.js's Local Development Server
.lintstagedrc.json:3
- Calling the global
lint
script here will lint the entire codebase rather than only staged files, leading to slower pre-commit checks. Use an ESLint command targeting staged files (e.g.,eslint --fix
) or restore a dedicatedlint:lint-staged
script that accepts file paths.
"node --run lint"
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7850 +/- ##
==========================================
+ Coverage 75.43% 75.44% +0.01%
==========================================
Files 101 101
Lines 8305 8305
Branches 218 218
==========================================
+ Hits 6265 6266 +1
+ Misses 2038 2037 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
I love that ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I particularly like calling into node_modules/.bin, but I don't see a good alternative if we're allergic to pnpm
Oh, did you want to update code owners to make sure all (ish) these files are covered by web-infra? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To humor @avivkeller and @bjohansebas — I'm not planning to create a "web-admin" team, as I don't think it's something we really need right now. I totally get that you're requesting changes, but just a heads up: the CODEOWNERS file isn't actually under the Website Team's ownership. Rather than dismissing your reviews, I'm just going to remove my handle and Brian’s from the file, since we’re not invested in this and I don’t want to hold up the PR over something that feels more like a formality. Hope that works! |
For the record, I was requesting changes over the workflow concerns, which also isn't really in the website team's scope, however, if merged like that, it would break production. Once that is resolved, my review is a non blocker |
That is a a-ok valid reason to request changes. My comment was more to highlight: OK I ack the request for changes, but technically speaking it holds no power (for CODEOWNERS), I might be wrong tho. Although I'd definitely want to honour it. |
I don't have the time to review the entire PR, so I'm going to dismiss my review so it doesn't block this PR any further, since my concern has already been addressed.
This PR updates all workflows and package.json's files by:
node --run
across the board instead ofpnpm
commands (which might help on situations where pnpm isn't in the env, and other scenarios) + removes internal dependency ofpnpm
(easier to switch to another package manager in the future)lint:lint-staged
commandcheck-types
as alint:types
as we're pretty much usingtsc
to "lint" the types.