Skip to content

fix(function): resolve nodePaths under an isolated store - #875

Merged
Kikobeats merged 1 commit into
masterfrom
fix/node-paths-pnpm
Aug 9, 2026
Merged

fix(function): resolve nodePaths under an isolated store#875
Kikobeats merged 1 commit into
masterfrom
fix/node-paths-pnpm

Conversation

@Kikobeats

@Kikobeats Kikobeats commented Aug 9, 2026

Copy link
Copy Markdown
Member

Problem

nodePaths tells isolated-function which dependencies a snippet requires are already on disk, so it can skip installing them (compile/index.jsdependencies filter).

It was derived by walking two levels up from @cloudflare/puppeteer:

const cloudflareDir = path.dirname(require.resolve('@cloudflare/puppeteer/package.json'))
const nodePaths = [path.resolve(cloudflareDir, '..', '..')]

That lands on the consumer's node_modules only under a flat (npm/yarn) install. Under pnpm it lands inside the isolated store:

nodePath: node_modules/.pnpm/@cloudflare+puppeteer@1.2.0/node_modules
entries:  @cloudflare @puppeteer debug devtools-protocol ws

which holds that package's own dependencies and nothing else. The check never matches, so every snippet installs a package that is already present.

Measured

Compiling a snippet that requires cheerio, clean tmpdir each round:

round 1  shipped install=414ms   fixed install=0ms
round 2  shipped install=403ms   fixed install=0ms
round 3  shipped install=402ms   fixed install=0ms

This repo installs with pnpm, so it is hitting this too — the existing interact with npm modules test reinstalls lodash, a devDependency of this package, on every run.

Fix

module.paths is the resolution chain Node itself would use from this file, so it reaches the consumer under either layout. The original entry is kept ahead of it, which leaves a flat install byte-identical.

Test

a dependency already present is not installed again asserts profiling.phases.install === 0 for a snippet requiring lodash.

  • without the fix: 420.096458 vs expected 0 — fails
  • with the fix: passes
  • full packages/function suite: 36 passed

Note

Low Risk
Localized change to how install skip paths are computed; no auth, security, or API surface changes.

Overview
Fixes redundant npm installs when user snippets require() packages that are already on disk under pnpm (and keeps flat npm/yarn behavior unchanged).

nodePaths passed to isolated-function is expanded from a single path (two levels up from @cloudflare/puppeteer) to a deduped list that still leads with that path and also includes module.paths, so dependency checks reach the consumer’s node_modules instead of stopping inside pnpm’s isolated store.

Adds a test that a snippet requiring lodash (already a devDependency) reports profiling.phases.install === 0.

Reviewed by Cursor Bugbot for commit dbdeb20. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Improved dependency resolution across different package installation layouts.
    • Prevented unnecessary reinstallation of dependencies that are already available locally.

`nodePaths` tells isolated-function which dependencies a snippet requires
are already on disk, so it can skip installing them. It was derived by
walking two levels up from `@cloudflare/puppeteer`, which lands on the
consumer's `node_modules` only under a flat install. With pnpm it lands
inside `node_modules/.pnpm/@cloudflare+puppeteer@1.2.0/node_modules`,
which holds that package's own dependencies and nothing else:

    entries: @cloudflare @puppeteer debug devtools-protocol ws

So the check never matched and every snippet paid a full install of a
package already present — measured at ~400ms per cold compile, three
rounds, consistent.

`module.paths` is the resolution chain Node itself would use from this
file, so it reaches the consumer under either layout. The original entry
is kept ahead of it, leaving a flat install byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YKVuh1R8HtB7SX7eKa7UsJ
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c2acfc79-e892-451d-8c79-0dd31a13a251

📥 Commits

Reviewing files that changed from the base of the PR and between 3ac7973 and dbdeb20.

📒 Files selected for processing (2)
  • packages/function/src/index.js
  • packages/function/test/index.js

📝 Walkthrough

Walkthrough

nodePaths now includes Node’s module resolution paths and the Cloudflare Puppeteer package root. A test confirms that an existing lodash dependency is reused without installation.

Changes

Dependency resolution

Layer / File(s) Summary
Resolve and validate local dependencies
packages/function/src/index.js, packages/function/test/index.js
nodePaths combines and deduplicates the Puppeteer package root with Node’s module paths. The test verifies that local lodash resolution reports zero installations.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing nodePaths resolution for isolated pnpm dependency stores.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/node-paths-pnpm

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 80.533% (-0.1%) from 80.629% — fix/node-paths-pnpm into master

@Kikobeats
Kikobeats merged commit 6308f0d into master Aug 9, 2026
21 of 23 checks passed
@Kikobeats
Kikobeats deleted the fix/node-paths-pnpm branch August 9, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants