Skip to content

fix: resolve vendor dependencies using node module resolution#691

Merged
rexxars merged 1 commit intomainfrom
fix/resolve-react-fix
Mar 16, 2026
Merged

fix: resolve vendor dependencies using node module resolution#691
rexxars merged 1 commit intomainfrom
fix/resolve-react-fix

Conversation

@rexxars
Copy link
Member

@rexxars rexxars commented Mar 16, 2026

Description

buildVendorDependencies resolved vendor entry points (react, react-dom, styled-components) using a naive node_modules/ relative path:

entry[chunkName] = resolve(`node_modules/${packageName}/${relativeEntryPoint}`)

This breaks in monorepos with hoisted dependencies (npm/yarn workspaces) where packages live in the root node_modules/ rather than the workspace's local one. The result is a Rollup error during sanity deploy:

RollupError: Could not resolve entry module "node_modules/react/cjs/react.production.js"

The fix extracts a getLocalPackageDir() function that uses import-meta-resolve to find the actual filesystem location of a package - the same resolution strategy getLocalPackageVersion already used for reading versions. This handles hoisted packages, pnpm symlinks, and other non-standard node_modules layouts.

Fixes #639 (comment)

What to review

  • getLocalPackageVersion.ts - the new getLocalPackageDir() function and the refactored getLocalPackageVersion() that now delegates to it. The fallback path for packages that don't export ./package.json is worth a look.
  • buildVendorDependencies.ts - the one-line change from resolve(...) to path.join(packageDir, relativeEntryPoint)
  • The test changes are mechanical (updated assertions since readPackageJson now receives a string path instead of a URL) plus new getLocalPackageDir tests.

Testing

Updated existing getLocalPackageVersion tests and added tests for getLocalPackageDir covering:

  • Direct package.json resolution (happy path)
  • Hoisted packages resolving to a parent node_modules/
  • Fallback when ./package.json isn't in the package exports map
  • Error propagation for missing packages

The monorepo scenario itself is hard to test in an automated way without setting up a real workspace fixture - the test mocks moduleResolve to return a hoisted path, which validates the logic but not the end-to-end resolution.

@rexxars rexxars requested a review from a team as a code owner March 16, 2026 22:38
@rexxars rexxars requested review from binoy14, cngonzalez and somerandomdude and removed request for a team March 16, 2026 22:38
@claude
Copy link

claude bot commented Mar 16, 2026

Claude finished @rexxars's task in 2m 51s —— View job


Review

1. Fallback path in getLocalPackageDir can return the wrong directory

packages/@sanity/cli/src/util/getLocalPackageVersion.ts lines 54–59:

const normalizedName = normalize(moduleName)
const idx = mainPath.lastIndexOf(normalizedName)

lastIndexOf finds the last occurrence of the package name in the full path. If the resolved entry file's basename contains the package name (common pattern — e.g. styled-components/dist/styled-components.browser.esm.js), the search matches inside the filename instead of the directory segment, returning …/dist/styled-components rather than …/dist/styled-components — wait, more precisely:

  • mainPath = /root/node_modules/styled-components/dist/styled-components.browser.esm.js
  • lastIndexOf('styled-components') → matches styled-components.browser.esm.js, not the directory
  • Result: /root/node_modules/styled-components/dist/styled-components ← wrong

The fix is to anchor the search to a directory segment:

const marker = `${path.sep}${normalizedName}${path.sep}`
const idx = mainPath.lastIndexOf(marker)
if (idx === -1) throw new Error(`Could not determine package directory for '${moduleName}'`)
return mainPath.slice(0, idx + marker.length - 1)

In practice this fallback only fires when a package doesn't export ./package.json, which none of the currently listed vendor packages do — but it's a real latent bug.

The test at line 216–229 uses @sanity/test/dist/index.js where the file name doesn't contain the package name, so it doesn't catch this case.


2. statSync can throw on permission errors in getExtractOptions.ts

packages/@sanity/cli/src/actions/schema/getExtractOptions.ts line 32:

const isExistingDirectory = existsSync(resolved) && statSync(resolved).isDirectory()

existsSync guards against ENOENT, but statSync will still throw EACCES if the path exists but the process lacks read permission. This is newly introduced surface area (the previous code never called statSync) and would crash the CLI unexpectedly. Wrap it:

let isExistingDirectory = false
try {
  isExistingDirectory = existsSync(resolved) && statSync(resolved).isDirectory()
} catch {
  isExistingDirectory = false
}

Fix this →

@rexxars rexxars enabled auto-merge (squash) March 16, 2026 22:38
@github-actions
Copy link

Coverage Delta

File Statements
packages/@sanity/cli/src/actions/build/buildVendorDependencies.ts 67.4% (+ 0.7%)
packages/@sanity/cli/src/actions/schema/getExtractOptions.ts 100.0% (±0%)
packages/@sanity/cli/src/util/getLocalPackageVersion.ts 94.7% (- 5.3%)

Comparing 3 changed files against main @ 293c5d8b2780c17d7d94e2b65b1b8d46fc1d2bd0

Overall Coverage

Metric Coverage
Statements 81.8% (- 0.0%)
Branches 70.7% (- 0.0%)
Functions 80.1% (+ 0.0%)
Lines 82.2% (- 0.0%)

@rexxars rexxars merged commit 44b0e98 into main Mar 16, 2026
41 checks passed
@rexxars rexxars deleted the fix/resolve-react-fix branch March 16, 2026 23:12
@squiggler-app squiggler-app bot mentioned this pull request Mar 16, 2026
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.

Deploy throws error after upgrade from 5.7 to 5.8

2 participants