Repository navigation
Support npm dependencies in saved packages - #306
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces npm dependency declaration and validation for saved Kody packages. It adds runtime dependency support where packages declare dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Package Source<br/>(with package.json)
participant Checks as Repo Checks
participant Graph as Module Graph<br/>Bundler
participant Assert as Dependency<br/>Assertion
participant Bundle as Runtime<br/>Bundle
participant Exec as Execution
Source->>Checks: Parse package.json<br/>for dependencies
Checks->>Checks: Extract declared<br/>dependencies
Checks->>Graph: Invoke buildKodyModuleBundle<br/>with sourceFiles
Graph->>Graph: Extract literal import<br/>specifiers from code
Graph->>Bundle: Create worker bundle
Bundle-->>Graph: Return modules
Graph->>Assert: Call assertion with<br/>declared dependencies
Assert->>Bundle: Check node_modules<br/>entries match
alt Dependencies Available
Assert-->>Graph: ✓ Pass
Graph-->>Checks: ✓ Bundle successful
Checks->>Exec: Artifact ready
else Missing Dependencies
Assert-->>Graph: ✗ Throw error
Graph-->>Checks: ✗ Bundle failed
Checks-->>Source: ✗ Repo check fails
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
May 1, 2026 03:11
Contributor
|
🔎 Preview deployed: https://kody-pr-306.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/package-runtime/module-graph.workers.test.ts (1)
47-47: ⚡ Quick winHarden the unresolved-import assertion pattern.
Line 47 only checks one import spelling, so equivalent unresolved forms can slip through undetected.
Proposed diff
- expect(moduleSources).not.toContain(`from "kleur"`) + expect(moduleSources).not.toMatch( + /\bfrom\s+['"]kleur['"]|\brequire\(\s*['"]kleur['"]\s*\)/, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/module-graph.workers.test.ts` at line 47, The test currently only checks for the exact string `from "kleur"` which misses other equivalent unresolved import forms; update the assertion on moduleSources (the variable used in the test) to assert it does not match a regex that covers variations (single or double quotes, any spacing, named/default imports) such as using a negative regex match instead of not.toContain; replace the expect(moduleSources).not.toContain(...) usage with an assertion that moduleSources does not match /from\s+['"]kleur['"]/ (or a slightly broader pattern to catch variations) so all unresolved-import forms for "kleur" are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/package-runtime/module-graph.workers.test.ts`:
- Line 47: The test currently only checks for the exact string `from "kleur"`
which misses other equivalent unresolved import forms; update the assertion on
moduleSources (the variable used in the test) to assert it does not match a
regex that covers variations (single or double quotes, any spacing,
named/default imports) such as using a negative regex match instead of
not.toContain; replace the expect(moduleSources).not.toContain(...) usage with
an assertion that moduleSources does not match /from\s+['"]kleur['"]/ (or a
slightly broader pattern to catch variations) so all unresolved-import forms for
"kleur" are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8578ef8f-03c7-48ef-aca5-cfe033db1df5
📒 Files selected for processing (14)
docs/contributing/packages-and-manifests.mddocs/use/packages.mdpackages/worker/src/jobs/service.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-runtime/import-specifiers.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/module-graph.workers.test.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/package-runtime/published-source-dependencies.tspackages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/repo-session-do.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 645095e. Configure here.
| remember(service.entry, true, 'module') | ||
| } | ||
| for (const subscription of listPackageSubscriptions(manifest)) { | ||
| remember(subscription.handler, true) |
Contributor
There was a problem hiding this comment.
Duplicate entry bundleKind not updated on path collision
Medium Severity
The remember helper in collectPackageTypecheckTargets merges includeStorage on path collision but silently drops the second bundleKind. If the same file path is registered as both an 'app' entry and a 'module' export/job/service, only the first bundleKind is retained. validatePackageBundles then only calls buildKodyAppBundle or buildKodyModuleBundle for that path — never both — so the other bundle kind is never validated during repo checks.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 645095e. Configure here.
| return `${scope}/${name}` | ||
| } | ||
| return specifier.split('/', 2)[0] ?? specifier | ||
| } |
Contributor
There was a problem hiding this comment.
Exported function getBarePackageNameFromSpecifier is never used
Low Severity
getBarePackageNameFromSpecifier is exported from import-specifiers.ts but is never imported or referenced by any other file in the codebase. It's dead code that was introduced in this PR.
Reviewed by Cursor Bugbot for commit 645095e. Configure here.
This was referenced May 10, 2026
This was referenced Jul 21, 2026
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
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.
Summary
package.json/Wrangler config and root-levelnode_modulesentries so@cloudflare/worker-bundlercan resolve declared npm dependencies for package exports, services, apps, jobs, and subscriptionspackage.jsonpresence, so unsupported or unresolved npm imports fail during checks instead of surfacing later at runtimepackage.jsondependency support and the publish/check-time resolution model for both users and contributorsTesting
npx vitest run --config vitest.node.config.ts packages/worker/src/repo/checks.node.test.ts packages/worker/src/package-invocations/service.node.test.tsnpx vitest run --config vitest.workers.config.ts packages/worker/src/package-runtime/module-graph.workers.test.tsnpx vitest run --config vitest.node.config.ts packages/worker/src/repo/checks.node.test.tsnpx vitest run --config vitest.workers.config.ts packages/worker/src/package-runtime/module-graph.workers.test.tsnpm run testnpm run test:e2e:run(via the successfulgit pushpre-push hook)Notes
Summary by CodeRabbit
Release Notes
Documentation
Improvements