build: convert monorepo from npm to pnpm - #570
Conversation
- pnpm-lock.yaml imported from package-lock.json; package-lock.json removed - workspaces moved to pnpm-workspace.yaml (linkWorkspacePackages so cross-package semver ranges keep resolving to the workspace copies) - npm overrides moved to pnpm-workspace.yaml overrides - patch-package replaced with pnpm patchedDependencies (css-tree, jsdom patches rewritten without the node_modules/ path prefix) - scripts, CI workflow (pnpm/action-setup@v6, cache: pnpm, frozen lockfile), husky pre-commit, and contributor docs converted to pnpm - @marko/runtime-tags publicly hoisted: marko's generated type extraction imports it from user code without a direct dependency (npm hoisting hid this) - marko deps bumped to ^5.39.25 (5.39.24 broke attr-tag typing snapshots; the old npm lockfile was stale and still pinned 5.39.11) - annotate language-tools marko processor export so d.ts emit does not reference a non-portable .pnpm path for magic-string - minimumReleaseAge disabled: repo consumes same-day marko releases - allowBuilds for esbuild, keytar, @vscode/vsce-sign
|
WalkthroughThe repository migrates package installation, scripts, CI jobs, release commands, hooks, and documentation from npm to pnpm. It adds pnpm workspace configuration, dependency overrides, package patches, and a pinned package-manager version. Marko dependencies are updated, the processor export is typed through a local constant, and local patches change JSON loading and synchronous XHR behavior. Lockfile ignore rules and existing test issue notes are also updated. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 19-32: Add a job-level read-only contents permission to the build
job covering the Install pnpm, Install node, package installation, build, and
lint steps in .github/workflows/ci.yml lines 19-32. Apply the same minimal
permissions to the test job at .github/workflows/ci.yml lines 43-54, while
leaving release job permissions unchanged so publishing retains required write
access.
In `@agent-feedback/bugs.md`:
- Line 5: Update the historical dependency description in the Marko snapshot
reproducibility note to reflect the PR’s current `^5.39.25` range, or explicitly
identify `^5.39.24` as the pre-migration manifest range; preserve the
explanation of the stale lockfile and affected fixtures.
In `@AGENTS.md`:
- Around line 90-91: Rename the CI job bullet in AGENTS.md from lint to build so
it matches the existing job name in the workflow, while preserving the lint
command and test job entry.
In `@package.json`:
- Line 13: Update the package.json format script to chain ESLint and Prettier
with failure propagation instead of an unconditional semicolon. Ensure Prettier
runs only when ESLint succeeds, preserving ESLint’s nonzero exit status so
dependent commands such as `@ci`:version do not continue after failure.
In `@pnpm-workspace.yaml`:
- Around line 17-18: Replace the global minimumReleaseAge: 0 setting in
pnpm-workspace.yaml with a minimumReleaseAgeExclude configuration that scopes
the release-age bypass only to Marko packages, preserving the release-age gate
for all other dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 863fe1a1-2a74-499e-bcd9-d9d925eae1c6
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (15)
.github/workflows/ci.yml.gitignore.husky/pre-commit.prettierignoreAGENTS.mdREADME.mdagent-feedback/bugs.mdpackage.jsonpackages/language-server/package.jsonpackages/language-tools/package.jsonpackages/language-tools/src/processors/marko.tspackages/ts-plugin/package.jsonpatches/css-tree@3.2.1.patchpatches/jsdom@29.1.1.patchpnpm-workspace.yaml
| - name: Install pnpm | ||
| uses: pnpm/action-setup@v6 | ||
| - name: Install node | ||
| id: node | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: lts/* | ||
| cache: npm | ||
| cache: pnpm | ||
| - name: Install packages (node@${{ steps.node.outputs.node-version }}) | ||
| run: npm ci | ||
| run: pnpm install --frozen-lockfile | ||
| - name: Run build | ||
| run: npm run @ci:build | ||
| run: pnpm run @ci:build | ||
| - name: Run lint | ||
| run: npm run @ci:lint | ||
| run: pnpm run @ci:lint |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict inherited GitHub Actions token permissions for non-release jobs.
Both jobs execute repository-controlled installation and tests without an explicit permissions block, so their token scope depends on repository defaults. Add a read-only contents permission for these jobs and retain write access only where release publishing requires it.
.github/workflows/ci.yml#L19-L32: add minimal permissions for the build job..github/workflows/ci.yml#L43-L54: add minimal permissions for the test job.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 14-32: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
📍 Affects 1 file
.github/workflows/ci.yml#L19-L32(this comment).github/workflows/ci.yml#L43-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 19 - 32, Add a job-level read-only
contents permission to the build job covering the Install pnpm, Install node,
package installation, build, and lint steps in .github/workflows/ci.yml lines
19-32. Apply the same minimal permissions to the test job at
.github/workflows/ci.yml lines 43-54, while leaving release job permissions
unchanged so publishing retains required write access.
Source: Linters/SAST tools
|
|
||
| Out-of-scope defects noticed while working on something else. Format and rules: [README.md](README.md). | ||
|
|
||
| - The `@marko/language-server` snapshot tests are not reproducible from the manifests alone: a fresh dependency resolution (delete lockfile + reinstall) picks up marko 5.39.25 / `@marko/runtime-tags` 6.3.16 and two fixtures fail against the committed snapshots — `attr-tags-params-js` and `for-tag` (in `packages/language-server/src/__tests__/fixtures/script/`). The previous `package-lock.json` masked this because it pinned marko 5.39.11 / runtime-tags 6.1.17 even though the manifests require `marko@^5.39.24` (the lock was stale relative to the ranges). Note marko 5.39.24 is worse still: with it, ~40 fixtures fail (attr-tag hoisting/bound-attr diagnostics like "Argument of type '{ section: never; }' is not assignable to parameter of type 'never'"); 5.39.25 fixed most of that upstream. The two remaining snapshot mismatches should be investigated against current marko and either fixed upstream or snapshotted. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale Marko version reference.
The PR updates Marko dependencies to ^5.39.25, but this note says the manifests require ^5.39.24. Update the historical explanation or explicitly label ^5.39.24 as the pre-migration manifest range so the diagnostic remains accurate.
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ... should be investigated against current marko and either fixed upstream or snapshotte...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent-feedback/bugs.md` at line 5, Update the historical dependency
description in the Marko snapshot reproducibility note to reflect the PR’s
current `^5.39.25` range, or explicitly identify `^5.39.24` as the pre-migration
manifest range; preserve the explanation of the stale lockfile and affected
fixtures.
| - **lint** job: `pnpm install --frozen-lockfile && pnpm run lint` | ||
| - **test** job: `pnpm install --frozen-lockfile && xvfb-run -a pnpm test` (Node 22 + 24 matrix) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the CI job name with the workflow.
.github/workflows/ci.yml runs lint in the build job, not a separate lint job. Rename this bullet to build or update the workflow if a distinct lint job is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 90 - 91, Rename the CI job bullet in AGENTS.md from
lint to build so it matches the existing job name in the workflow, while
preserving the lint command and test job entry.
| "build": "tsc -b && NODE_ENV=production pnpm -r run build", | ||
| "build:dev": "tsx ./packages/vscode/build.mts", | ||
| "change": "changeset add", | ||
| "format": "eslint --fix .; prettier . --write --log-level=warn", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve ESLint’s exit status in format.
Using ; allows Prettier to overwrite ESLint’s failure status, so the command can succeed even when ESLint fails. This also lets @ci:version continue after an unsuccessful lint fix.
Proposed fix
- "format": "eslint --fix .; prettier . --write --log-level=warn",
+ "format": "eslint --fix . && prettier . --write --log-level=warn",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "format": "eslint --fix .; prettier . --write --log-level=warn", | |
| "format": "eslint --fix . && prettier . --write --log-level=warn", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 13, Update the package.json format script to chain
ESLint and Prettier with failure propagation instead of an unconditional
semicolon. Ensure Prettier runs only when ESLint succeeds, preserving ESLint’s
nonzero exit status so dependent commands such as `@ci`:version do not continue
after failure.
| # This repo depends on same-day marko releases; disable pnpm's release-age gate. | ||
| minimumReleaseAge: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## pnpm-workspace.yaml\n'
cat -n pnpm-workspace.yaml
printf '\n## package.json files mentioning versioning or lockfile resolution\n'
rg -n '"version|changeset|release|lockfile|pnpm"' -g 'package.json' -g '!**/node_modules/**' .
printf '\n## workspace-related config files\n'
fd -a -t f 'pnpm-workspace.yaml|.npmrc|package.json|.changeset' .Repository: marko-js/language-server
Length of output: 2309
🌐 Web query:
pnpm minimumReleaseAge minimumReleaseAgeExclude documentation
💡 Result:
The minimumReleaseAge and minimumReleaseAgeExclude settings in pnpm are security features designed to mitigate supply chain attacks by delaying the installation of newly published package versions [1][2]. minimumReleaseAge This setting defines the minimum number of minutes that must pass after a package version is published before pnpm will allow its installation [3][4]. It applies to both direct and transitive dependencies [3][5]. - Configuration: It is configured in pnpm-workspace.yaml (using camelCase: minimumReleaseAge) or via global configuration files/environment variables (using kebab-case: minimum-release-age) [3][1]. - Default: As of pnpm v11, the default value is 1440 (1 day) [2][6]. In versions prior to v11, the default was 0 [3]. - Opt-out: You can disable this restriction by setting the value to 0 [2][6]. minimumReleaseAgeExclude This setting allows you to bypass the minimumReleaseAge restriction for specific dependencies, ensuring they can be installed immediately regardless of their publication time [3][4]. - Usage: It accepts an array of strings [3]. You can specify exact package names, glob patterns (e.g., '@myorg/'), or specific versions (e.g., 'nx@21.6.5' or 'webpack@4.47.0 || 5.102.1') [3][1]. - Integration: When running pnpm audit --fix, pnpm may automatically add patched versions of vulnerable dependencies to this list to ensure security fixes can be applied immediately [7]. Example Configuration (pnpm-workspace.yaml): minimumReleaseAge: 1440 minimumReleaseAgeExclude: - webpack - '@myorg/' - react@18.3.1 || 19.0.0
Citations:
- 1: https://charpeni.com/blog/protecting-against-compromised-packages-with-minimum-release-age
- 2: https://pnpm.io/supply-chain-security
- 3: https://pnpm.io/settings
- 4: https://pnpm.io/blog/releases/10.16
- 5: Add a way to enforce a minimum package age policy pnpm/pnpm#9921
- 6: feat: set default minimumReleaseAge to 1 day (1440 minutes) pnpm/pnpm#11158
- 7: https://pnpm.io/cli/audit
Avoid disabling minimumReleaseAge globally. minimumReleaseAge: 0 turns off pnpm’s release-age gate for every newly resolved dependency. If the intent is only to allow same-day Marko releases, use minimumReleaseAgeExclude to scope that exception instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pnpm-workspace.yaml` around lines 17 - 18, Replace the global
minimumReleaseAge: 0 setting in pnpm-workspace.yaml with a
minimumReleaseAgeExclude configuration that scopes the release-age bypass only
to Marko packages, preserving the release-age gate for all other dependencies.
1 participant
Converts the repo from npm workspaces to pnpm (following marko-js/marko#3514).
pnpm-lock.yamlimported frompackage-lock.json; workspaces + overrides moved topnpm-workspace.yaml(linkWorkspacePackages: true)patchedDependencies(css-tree, jsdom patches rewritten without thenode_modules/prefix; verified applied in the store)--ws→pnpm -r,-w→--filter), CI (pnpm/action-setup@v6,cache: pnpm, frozen lockfile), husky hook, and contributor docs converted; consumer-facing npm install instructions left as-is@marko/runtime-tagsis publicly hoisted: marko's extracted type output imports@marko/runtime-tags/tags/*.d.markofrom user code without it being a declared dependency — npm's hoisting hid this, and without the hoist 40 language-server fixtures fail. It is intentionally not a direct dep of@marko/language-server, since that flips translator detection to tags-mode^5.39.25: the oldpackage-lock.jsonwas stale (pinned marko 5.39.11 despite^5.39.24ranges), and 5.39.24 breaks ~40 attr-tag typing snapshots; 5.39.25 fixes themminimumReleaseAge: 0since this repo consumes same-day marko releasesProcessorConfigso d.ts emit doesn't reference a non-portable.pnpmmagic-string pathVerified locally: clean
pnpm install --frozen-lockfile, full build, lint, 189/189 language-server tests.vsce packageworks unchanged (release.mts's root-package.json rename trick still applies; the packaged vsix includes themodules/marko-ts-pluginshim). vscode extension tests fail 7/17 both before (npm) and after (pnpm) — pre-existing VS Code Insiders issue, recorded in agent-feedback along with the stale-lockfile finding.