build: switch workspace metadata to pnpm workspaces - #223
Merged
Conversation
🦋 Changeset detectedLatest commit: 86523bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe repository migrates workspace tooling from npm to pnpm. It adds pnpm workspace configuration, pins pnpm, updates package dependencies and scripts, and changes CI and pre-commit commands. Lockfile-related ignore and formatting settings are aligned with 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🤖 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-31: Declare least-privilege GITHUB_TOKEN permissions by setting
contents: read at workflow scope or explicitly on both build and test jobs in
ci.yml. Keep the existing write permissions only on the release job. Record this
out-of-scope security finding in agent-feedback/ before completing the change.
In `@agent-feedback/dx.md`:
- Around line 5-10: Clarify the cleanup guidance for the testPage helper by
identifying the specific missing behavior when the spawned preview process fails
during startup: ensure the process and its socket are fully terminated before
returning, including force-killing it or applying a close timeout if graceful
cleanup hangs. Do not duplicate the existing server.close() cleanup; update the
recommendation to target startup-failure cleanup and prevent later fixtures from
inheriting the failed process state.
In `@package.json`:
- Line 13: Update the package.json format script so ESLint failures are
preserved by chaining the ESLint and Prettier commands with && or explicitly
combining their exit statuses. Keep both formatting steps enabled while ensuring
the script exits unsuccessfully whenever ESLint fails.
🪄 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: 0cd4eb45-9f52-4057-8436-bf52a6d0f50c
⛔ 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 (13)
.changeset/tidy-pnpm-workspaces.md.gitattributes.github/workflows/ci.yml.gitignore.husky/pre-commit.prettierignoreAGENTS.mdagent-feedback/dx.mdcspell.jsonpackage.jsonpackages/adapters/static/package.jsonpackages/run/package.jsonpnpm-workspace.yaml
Under pnpm's isolated node_modules, packages/run declared the netlify, node, and static adapters as devDependencies so fixtures could import them. But resolveAdapter() scans the nearest package.json's dependencies and auto-selects the first @marko/run-adapter* it finds, so every adapter-less fixture started resolving to the netlify adapter instead of the default adapter -- changing preview behavior and breaking the config-override, not-handled, and verb-specific-js-non-object-meta snapshots. Move the adapter devDependencies to the workspace-root package.json. They stay resolvable from the fixture tree via node's upward lookup (and adapter-specific fixtures still import them explicitly), while packages/run no longer advertises them to resolveAdapter, so generic fixtures fall back to the default adapter as they do on main.
The micro-frame-fetch preview server crashed at startup under pnpm with `TypeError: __toCommonJS(lru_cache_exports) is not a constructor`. The @micro-frame/marko dependency ships uncompiled .marko files, so the Marko plugin bundles it into the SSR server, and its transitive make-fetch-happen -> cacache -> lru-cache@7 chain got bundled with it. Under pnpm, make-fetch-happen is private to @micro-frame/marko and not resolvable from the app root, so Vite cannot externalize it; rolldown then bundles lru-cache@7's dual-published ESM build for cacache's CJS require(), whose interop wrapper is not a constructor. On npm's flat node_modules the chain externalizes and Node loads lru-cache's CJS build, which is why it only failed under pnpm. Hoist make-fetch-happen via publicHoistPattern so Vite externalizes it as it does on npm, keeping the CJS chain out of the SSR bundle. The general build behavior is recorded in agent-feedback/bugs.md.
…ons findings - bugs.md: @marko/run can bundle a dependency's private transitive CJS deps under pnpm and crash on ESM/CJS interop -- the general case behind the micro-frame-fetch fix (a pnpm consumer with a .marko-shipping package that has a private dual-published CJS transitive dep hits the same crash). - dx.md: record least-privilege permissions for the CI build/test jobs (flagged by CodeRabbit/zizmor), and correct the Netlify preview-test note now that the adapter-resolution fix removed the cascade it described.
The build and test jobs declared no permissions block, so their GITHUB_TOKEN inherited the repository/organization default scope. Add a workflow-level `permissions: contents: read`; the release job keeps its own id-token/contents/pull-requests write block, which fully overrides the workflow default for that job. Resolves the excessive-permissions finding (zizmor/CodeRabbit), so the corresponding agent-feedback note is removed.
The publicHoistPattern entries are workarounds whose rationale was only recorded in agent-feedback; comment them inline so a future cleanup knows what each one unblocks before removing it.
…ependency Shrink publicHoistPattern to the one entry that genuinely needs hoist semantics. make-fetch-happen only needs to be resolvable from the repo root so Vite externalizes @micro-frame/marko's fetch chain in SSR test builds; declaring it as a root devDependency does that honestly, and pnpm dedupes it to the same instance @micro-frame/marko resolves. @marko/runtime-tags stays hoisted because it must be resolvable yet UNDECLARED: listing it in a package.json on the fixtures' lookup path flips marko 5's api detection and changes fixture rendering (verified -- declaring it broke seven fixtures' snapshots), while npm's flat layout provided exactly the resolvable-but-undeclared shape. Document that constraint inline.
rturnq
force-pushed
the
claude/rebase-commits-attribution-a01aq5
branch
from
July 30, 2026 22:33
89e20ea to
86523bc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Migrates the monorepo's package management and workspace metadata from npm to pnpm workspaces.
build: switch workspace metadata to pnpm— replacespackage-lock.jsonwithpnpm-lock.yaml, addspnpm-workspace.yaml, and updates the tooling that referenced npm: rootpackage.json,.github/workflows/ci.yml,.husky/pre-commit,.prettierignore,.gitignore,.gitattributes,cspell.json, andAGENTS.md. Also declares the direct dependencies the Vite integration needs under pnpm's strict resolution —@marko/compiler,@oxc-project/types, androlldownon@marko/run, and@types/compression(dev) on@marko/run-adapter-static.test: support pnpm dependency isolation— adds the workspace adapters (@marko/run-adapter-netlify,-node,-static) andmarkoas dev dependencies of@marko/runso the test suite resolves them under pnpm's isolatednode_modules.docs: record Netlify test setup friction— records the Netlify test-setup friction encountered during the migration inagent-feedback/dx.md.A changeset (
.changeset/tidy-pnpm-workspaces.md) bumps@marko/runat patch, covering the new direct runtime dependencies now declared on the published package. The other additions are dev-only and don't affect consumers.Motivation and Context
Under npm's hoisting, packages could resolve dependencies they never declared. pnpm installs with an isolated
node_modules, which surfaces those undeclared dependencies as resolution failures. Moving the workspace to pnpm and declaring the previously-hoisted dependencies makes the dependency graph explicit and the install reproducible.Screenshots (if appropriate):
N/A — build/tooling change.
Checklist:
Generated by Claude Code