-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
pnpmtonode --runornpx - Alphabetizes file lists in
package.jsonand repositions license/type fields - Introduces
devEnginesand removes customlint:lint-staged&check-typescommands
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
onlyBuiltDependencieslist 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
lintscript 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-stagedscript 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 ! |
MattIPv4
left a comment
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? |
bjohansebas
left a comment
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 --runacross the board instead ofpnpmcommands (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-stagedcommandcheck-typesas alint:typesas we're pretty much usingtscto "lint" the types.