fix(npm): extract each package under node_modules/<name> in its repo - #159
Open
mikn wants to merge 1 commit into
Open
fix(npm): extract each package under node_modules/<name> in its repo#159mikn wants to merge 1 commit into
mikn wants to merge 1 commit into
Conversation
npm_import extracted a package at the root of its external repository, so every path the ruleset wrote for it -- a tsconfig `paths` value, an action input, an exec path -- lacked a node_modules segment. TypeScript classifies a `paths` match by that segment alone: under one the file is a library file, type-checked and never emitted; under none it is project source, emit-eligible and checked against rootDir. The .d.ts files the sandbox stages today are exempt either way, which is why nothing failed on it; a .ts reached through `paths` is TS6059 the moment a package's module entry is one (rt-evidence/logs/inv7/wide-event-sink-ts2306, replica-r1 against replica-r1c). The tarball is still extracted and patched at the repository root, where rctx.patch applies, and then moved under node_modules/<name>/ through a staging directory, since a package can ship a node_modules of its own (@parcel/watcher-wasm). A BUILD, WORKSPACE, MODULE.bazel or REPO.bazel shipped at the package root is deleted: at the repository root the generated BUILD file overwrote it, and one level down Bazel would read it as a boundary and the glob would return nothing. The stanza writes package_dir, package_files, exports_types and exports_files under that directory. Every consumer derives its paths from package_dir.dirname, so ts_npm_package, ts_compile, tsconfig_aspect and node_modules are unchanged; the node_modules tree and the editor's .bazel/npm/<name>/ copies are laid out from the package root as before, and `bazel run //:refresh_tsconfig` regenerates the checked-in tsconfig.json with no diff. The tree no longer carries each package's generated BUILD.bazel and REPO.bazel, which the root glob had matched. //tests/workers:library_paths_test asserts every `paths` value written for @cloudflare/workers-types carries /node_modules/@cloudflare/workers-types/; the written-form test in tests/npm/exports_types_tests.bzl asserts the three labels the stanza writes; the tests that matched an exec path by repository name alone -- link_depth, precedence, package_root, exports_pattern, config_agreement and the Go tests over runfiles -- expect node_modules/<name>/ after it. Red on 2bcc23f with the tests in the tree and the .bzl untouched (rt-evidence/logs/fix8r/J1/01-red.log): $ bazelisk --output_base=/var/tmp/fix8-J1-ob test --noshow_progress --nocache_test_results --local_test_jobs=2 --test_summary=short --test_output=errors --keep_going //tests/npm:exports_types_tests //tests/npm:type_references_tests //tests/npm:link_depth_tests //tests/workers:library_paths_test //tests/npm_types_barename:types_package_root_test //tests/npm_subpath:wildcard_root_test //tests/npm_exports_pattern:pattern_paths_test //tests/npm:npm_patched_dependency_test //tests/npm:npm_platform_independent_graph_test //tests/npm:npm_tarball_prefix_test [... 5 lines ...] In test _written_form_test from //tests/npm:exports_types_tests.bzl: vite@8.2.2: exports_types names a file under the package root, not a repository (Expected "node_modules/vite/dist/node/index.d.ts", but got "dist/node/index.d.ts") [... 192 lines ...] In test _library_paths_impl from //tests/workers:library_paths_tests.bzl: @cloudflare/workers-types names a file outside node_modules, which TypeScript takes for project source: ../../../../../external/+npm+npm_workers__cloudflare_workers-types__5_20260825_1/index.d.ts In test _library_paths_impl from //tests/workers:library_paths_tests.bzl: @cloudflare/workers-types/* names a file outside node_modules, which TypeScript takes for project source: ../../../../../external/+npm+npm_workers__cloudflare_workers-types__5_20260825_1/* [... 44 lines ...] Executed 17 out of 17 tests: 9 tests pass and 8 fail locally. [... 2 lines ...] # exit=3 Green on this tree (rt-evidence/logs/fix9r/J1/02-green.log): $ bazelisk --output_base=/var/tmp/fix9-J1-ob test --noshow_progress --nocache_test_results --local_test_jobs=2 --test_summary=short --test_output=errors --keep_going //tests/npm:exports_types_tests //tests/npm:type_references_tests //tests/npm:link_depth_tests //tests/workers:library_paths_test //tests/npm_types_barename:types_package_root_test //tests/npm_subpath:wildcard_root_test //tests/npm_exports_pattern:pattern_paths_test //tests/npm:npm_patched_dependency_test //tests/npm:npm_platform_independent_graph_test //tests/npm:npm_tarball_prefix_test //tests/npm:npm_multi_version_test //tests/npm_types_barename:test_config_agreement //tools/changelog:changelog_test //:refresh_tsconfig_test [... 9 lines ...] //:refresh_tsconfig_test PASSED in 0.0s [... 18 lines ...] //tests/workers:library_paths_test PASSED in 0.0s [... 2 lines ...] Executed 21 out of 21 tests: 21 tests pass. [... 2 lines ...] # exit=0 Full suite on tree b72d8a1, this change on 2bcc23f (fix8r/J1/14-suite.log). The tree here is that one rebased onto eb947cd, `git range-diff` `=` (fix8r/restack/08-rebase-J1.log), plus comment, docstring and docs hunks in six files (fix9r/J1/11-polish-diff.log): $ bazelisk --output_base=/var/tmp/fix8-J1-ob test --noshow_progress --nocache_test_results --local_test_jobs=2 //... [... 5 lines ...] INFO: Build completed successfully, 372 total actions [... 1 lines ...] Executed 371 out of 371 tests: 371 tests pass. [... 2 lines ...] # exit=0
This was referenced Sep 6, 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.
npm_importextracted a package at the root of its external repository (_npm_import_impl,npm/private/npm_import.bzl), so every path the ruleset wrote for it -- a tsconfigpathsvalue, an action input, an exec path -- lacked anode_modulessegment. TypeScript classifies apathsmatch by that segment alone: under one the file is a library file, type-checked and never emitted; under none it is project source, emit-eligible and checked againstrootDir. The files the sandbox stages for an npm package are its.d.ts, itspackage.jsonand its.json(tsgo_inputs,ts/private/ts_compile.bzl), which are exempt either way, so nothing failed on the layout by itself; a.tsreached throughpathsisTS6059the moment a package's module entry is one. That is the shape behind the trial's//packages/workers-wide-event-sink:workers-wide-event-sink, oneTS2306onimport type { TraceItem, TraceLog } from "@cloudflare/workers-types"because the pin names the package's global-scriptindex.d.ts(logs/trial-7/measure/scoped.log:17): pinning the module-formindex.tsinstead trades theTS2306forTS6059in a node_modules-free replica of the program (logs/inv7/wide-event-sink-ts2306/replica-r1-paths-index-ts-rootdir.log), and the same pin under anode_modules/@cloudflare/workers-types/directory is 0 errors (replica-r1c-node-modules-segment-index-ts.log). This PR is the layout half; the entry half (J2) stages the module entry and pinspathsto it.The tarball is still extracted and patched at the repository root, where
rctx.patchapplies, and then moved undernode_modules/<name>/through a staging directory (_move_package), since a package can ship anode_modulesof its own --@parcel/watcher-wasm@2.3.0and2.6.0, each holdingnode_modules/napi-wasm, are the 2 of the 513 npm repositories fetched into the trial's output base1de19487761e9cf1e53b4aca66ee44ffunder the old layout that do (logs/fix9r/J1/01-parcel-nested-node-modules.log) -- and a directory cannot be renamed into one of the same name. ABUILD,BUILD.bazel,WORKSPACE,WORKSPACE.bazel,MODULE.bazelorREPO.bazelshipped at the package root is deleted: at the repository root the generated BUILD file overwrote it, and one level down Bazel reads it as a package or repository boundary and the glob returns nothing. The stanza writespackage_dir = ":node_modules/<name>/package.json",package_files = glob(["node_modules/<name>/**/*"], ...),exports_typesunder the same directory, andexports_files(["node_modules/<name>/package.json"]); the manifest is read and everyhas_file/readpredicate answers under that root (logs/fix8r/J1/10-layout.logshows the generated files for zod,@cloudflare/workers-types,@types/culoriand nanoid). Every consumer already derives its paths fromNpmPackageInfo.package_dir.dirname--ts_npm_package'spackage_root,ts_compile'spathswriter,tsconfig_aspect's installed tree,node_modules' copy manifest -- so none of them changes. The editor's.bazel/npm/<name>/copies are laid out from the package root as before:bazel run //:refresh_tsconfigregenerates the checked-intsconfig.jsonwith no diff (09-refresh-tsconfig.log,git status --shortafter the run lists only the then-uncommitted edits of this PR andgit diff --stat -- tsconfig.jsonprints nothing), and//:refresh_tsconfig_testpasses without it. Thenode_modulestree is laid out from the package root too, and the scoped glob no longer matches each package's generatedBUILD.bazelandREPO.bazel:bazel-bin/tests/js_entry/generator_node_modules/nanoidlists 14 entries on this output base against 16 on one built before the change, the two being those files (12-tree-and-markers.log).Docs:
docs/guides/npm.md§ One Repository per Package gains the paragraph on where a package sits and what the segment means to TypeScript;docs/rules/ts-compile.md§ Action Inputs gains the sentences on where a value naming an npm package points, and that the map's other values -- amodule_name, apath_aliasesprefix, a workspace member -- are workspace paths (on the skeptic's output base, 10401 of the 10505 values in 213 generated tsconfigs point intoexternal/, every one with the segment, and the other 104 across 26 keys are such paths:logs/fix8r/fix-J1-skeptic/10-paths-scan.log,10b-paths-nonexternal.log);AGENTS.md§ npm Internals names the layout and that consumers derive frompackage_dir.dirname; the module docstring ofnpm/private/npm_bin.bzlshows the stanza as it is now written, and the comment at its suffix match the path it matches.changelog.d/npm-node-modules-layout.mdrecords it under### Changed, exec paths included: a test or script that matched an exec path by repository name alone has to expectnode_modules/<name>/after it.Fixture
//tests/workers:library_paths_test(tests/workers/library_paths_tests.bzl) reads the tsconfigts_compilewrites for:experimental_types, whose one dep is@npm_workers//:cloudflare_workers-types, and asserts every value of the@cloudflare/workers-typesand@cloudflare/workers-types/*keys carries/node_modules/@cloudflare/workers-types/. The written-form test intests/npm/exports_types_tests.bzlasserts, over the 14 manifests in its table that designate a declaration, that the stanza writesexports_typesas a file undernode_modules/<name>/,package_diras the manifest there, andpackage_filesas the glob over that directory alone; thepackage_stanzacalls in it and intype_references_tests.bzlpasspackageon the attrs struct, which the stanza now reads. The tests that matched an exec path by repository name alone are moved to the layout:tests/npm_types_barename/precedence_tests.bzl(culoriandculori/*end at/node_modules/@types/culori),tests/npm_subpath/package_root_tests.bzl(the wildcard's first value ends at/node_modules/<name>),tests/npm_exports_pattern/exports_pattern_tests.bzl(unenv's second value is/node_modules/unenv/*),tests/npm_types_barename/config_agreement_test.mjs(the build route'ssplittakes the package asnode_modules/<name>inside the repository, one segment or two, and compares what is under it), and the four Go tests over runfiles (npm_patched_dependency_test.go,npm_platform_independent_graph_test.go,npm_tarball_prefix_test.go,npm_multi_version_test.go), whoseFoundFile/FoundDirpatterns name the two segments;tests/npm/link_depth_tests.bzl's three exec-path strings are rewritten for truth, as the skeptic noted, though that test reads only the package-relative half and passed on either layout.tests/npm_types_shim/shim_tests.bzlandtests/workers/shipped_subpath_tests.bzlmatch a repository name as a path segment or a file suffix and are unchanged. No behavioural fixture is possible in this PR alone: no.tsof an npm package reaches the sandbox until J2 stages one, so theTS6059shape has nothing to fail on here; J2'sts_compileover the module import is that fixture.Red on 2bcc23f with the tests in the tree and the
.bzluntouched, tree 69a44bd (logs/fix8r/J1/01-red.log): 8 of the 17 tests fail, the nine controls in the three suites pass.Green on this tree, 6824a23 (
logs/fix9r/J1/02-green.log),//tests/npm:npm_multi_version_test,//tests/npm_types_barename:test_config_agreement,//tools/changelog:changelog_testand//:refresh_tsconfig_testadded to the set:The first full suite (
11-suite.log, tree f78a6e9) found the two tests the targeted set had missed://tests/npm:npm_multi_version_test(FoundDir("*rolldown_pluginutils__1_0_1")thenpackage.jsonin it) and//tests/npm_types_barename:test_config_agreement(116resolves to a different file in each configlines, every one the build'snode_modules/<name>/against the editor's bare path),Executed 371 out of 371 tests: 369 tests pass and 2 fail locally.Both moved to the layout, the two pass (13-green-two.log,Executed 2 out of 2 tests: 2 tests pass.) and the suite on tree b72d8a1, this change on 2bcc23f (14-suite.log):bazelisk build --config=ci //... --output_groups=+_validationcompletes on tree b72d8a1 (15-ci-build-validation.log, exit 0) and at d3c9520, the restacked stack's tip (logs/fix8r/restack/13-ci-build-validation.log, exit 0). On this tree, 6824a23:buildifier --mode=check -r .,gofmt -l .andgo vetover CI's package list exit 0 (logs/fix9r/J1/03-lint-buildifier.log,04-lint-gofmt.log,05-lint-govet.log);uvx --from mkdocs-material mkdocs build --strictbuilds the site (06-lint-mkdocs.log, exit 0);tools/ci/check_test_sources.sh(134 test source files, all claimed by a test target; 6 manual-only, allowlisted) andtools/ci/check_integration_shards.sh(npm 5, core 10) exit 0 (07-check_test_sources.log,08-check_integration_shards.log).bazel run //gazelle -- -mode=diffon tree b72d8a1 prints 595 lines, as on the stack tip (logs/fix8r/P/05-modediff.stdout); the two lines that differ are a context line and a hunk header inside theasset_libraryproposal Gazelle already makes fortests/workers/BUILD.bazel, moved down by the six lines this PR adds to that file (17-modediff.log). Of the 531 npm repositories the suite fetched into output basefix8-J1-ob, none ships anode_modulesof its own and none a boundary file at its package root (12-tree-and-markers.log), so the staging move and the deletion run on every fetch and the cases they exist for are pinned by no fixture here.This head, ad8a66c, is 94c641f -- 870658d rebased onto eb947cd with the patch unchanged,
git range-diffprints=(logs/fix8r/restack/08-rebase-J1.log) -- amended with comment, docstring and docs hunks in six files, 14 insertions and 18 deletions (git diff --stat 94c641f ad8a66c,logs/fix9r/J1/09-diffstat.log; the seven hunks are in11-polish-diff.log), and the subject shortened to 68 characters. The full suite at d3c9520, J2 on 94c641f, is in #160's body (logs/fix8r/restack/12-suite.log, 380 of 380).On the trial
Nothing in lov-paraglide is edited by this PR. Measured at cae299e with the override at 2bcc23f,
//packages/workers-wide-event-sink:workers-wide-event-sinkreports oneerror TSline, theTS2306above (logs/trial-7/measure/scoped.log:17). With the override at ad8a66c alone that line is expected to stay: the pin still namesindex.d.ts, and the segment changes what TypeScript does with apathshit that is a.ts, of which the sandbox holds none until J2. No diagnostic count is expected to move from this half. What the trial will see is everypathsvalue in every generated tsconfig carryingnode_modules/<name>/, exec paths two segments longer (external/+npm+npm__zod__4_1_5/node_modules/zod/index.d.ts),node_modulestrees without each package'sBUILD.bazelandREPO.bazelcopy, and@parcel/watcher-wasm's shippednode_modulesgoing through the staging move; whether any script in the monorepo matched an exec path by repository name is the trial's to find. With J2 on top, theindex.tspin for@cloudflare/workers-typesis expected to resolve as a library file, replica-r1c's 0 errors rather than replica-r1'sTS6059.Base branch:
mikn/ts-test-untyped-packages(1edaa40); head a1fb2e7, the polished commit ad8a66c rebased onto it with its patch unchanged (logs/fix9r/restack/restack-mechanical.log).