Fix ESM <> CJS dual-package hazard determinism bug - #22231
Conversation
|
Updated 2:05 AM PT - Aug 30th, 2025
❌ @Jarred-Sumner, your commit 79ea473 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 22231That installs a local version of the PR into your bun-22231 --bun |
| secondary != path and | ||
| !strings.eqlLong(secondary.text, path.text, true)) | ||
| { | ||
| const secondary_path_to_copy = secondary.dupeAlloc(this.allocator()) catch |err| bun.handleOom(err); |
There was a problem hiding this comment.
| const secondary_path_to_copy = secondary.dupeAlloc(this.allocator()) catch |err| bun.handleOom(err); | |
| const secondary_path_to_copy = bun.handleOom(secondary.dupeAlloc(this.allocator())); |
| } | ||
|
|
||
| const resolve_entry = bun.handleOom(resolve_queue.getOrPut(hash_key)); | ||
| const resolve_entry = resolve_queue.getOrPut(path.text) catch |err| bun.handleOom(err); |
There was a problem hiding this comment.
| const resolve_entry = resolve_queue.getOrPut(path.text) catch |err| bun.handleOom(err); | |
| const resolve_entry = bun.handleOom(resolve_queue.getOrPut(path.text)) |
|
|
||
| if (is_html_entrypoint) { | ||
| this.generateServerHTMLModule(path, target, import_record, hash_key) catch unreachable; | ||
| this.generateServerHTMLModule(path, target, import_record, path.text) catch unreachable; |
There was a problem hiding this comment.
| this.generateServerHTMLModule(path, target, import_record, path.text) catch unreachable; | |
| bun.handleOom(this.generateServerHTMLModule(path, target, import_record, path.text)); |
| var new_input_file = Graph.InputFile{ | ||
| .source = Logger.Source.initEmptyFile(new_task.path.text), | ||
| .side_effects = value.side_effects, | ||
| .secondary_path = if (value.secondary_path_for_commonjs_interop) |*secondary_path| secondary_path.text else "", |
There was a problem hiding this comment.
what does it mean when secondary_path == ""?
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
|
Warning Rate limit exceeded@Jarred-Sumner has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughSwitches path→index maps from hash keys to string paths, adds a PathToSourceIndexMap module, introduces per-input Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Bundle as BundleV2
participant Resolver as Resolver
participant PTI as PathToSourceIndexMap
participant Graph as Graph
CLI->>Bundle: enqueueEntryItem(resolve, is_entry_point, target)
Bundle->>Resolver: resolve import
Resolver-->>Bundle: (path.text, ?secondary_path)
Bundle->>PTI: getOrPut(path.text)
PTI-->>Bundle: source_index
Bundle->>Graph: record InputFile { path, secondary_path? }
Note over Bundle,Graph: graph.has_any_secondary_paths set if any secondary_path present
sequenceDiagram
autonumber
participant Bundle as BundleV2
participant PTI as PathToSourceIndexMap
participant LG as LinkerGraph
alt graph.has_any_secondary_paths
Bundle->>Bundle: scanForSecondaryPaths()
loop import_record
Bundle->>PTI: get(import_record.secondary_path.text)
alt found
Bundle->>LG: rewrite import_record.source_index
else
Note right of Bundle: leave import_index unchanged
end
end
else
Note right of Bundle: skip scanForSecondaryPaths
end
sequenceDiagram
autonumber
participant Dev as DevServer
participant IG as IncrementalGraph
participant PTI as PathToSourceIndexMap
Dev->>IG: onFileDeleted(abs_path)
loop each build_graph.map
IG->>PTI: remove(abs_path.text)
alt removed
Note right of IG: entry cleared
else
Note right of IG: no-op
end
end
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/bundler/PathToSourceIndexMap.zig (3)
18-20: Enforce absolute-file-path invariant at the map boundaryAdd a CI-only assertion to fail fast if a non-absolute file path is inserted/queried via Path. This aligns with the new Fs.Path.assertFilePathIsAbsolute and prevents accidental relative-keying.
Apply this diff:
pub fn putPath(this: *PathToSourceIndexMap, allocator: std.mem.Allocator, path: *const Fs.Path, value: Index.Int) bun.OOM!void { + path.assertFilePathIsAbsolute(); try this.map.put(allocator, path.text, value); } pub fn getOrPutPath(this: *PathToSourceIndexMap, allocator: std.mem.Allocator, path: *const Fs.Path) bun.OOM!Map.GetOrPutResult { - return this.getOrPut(allocator, path.text); + path.assertFilePathIsAbsolute(); + return this.getOrPut(allocator, path.text); }Also applies to: 26-28
3-6: Comment mismatch with implementationThe comment says keys are “not owned by this map / arena-allocated,” but the type is a StringHashMapUnmanaged, which typically allocates and owns copies via the provided allocator. Either switch to the unowned variant or reword the comment to avoid implying zero-copy storage.
42-47: Remove unused std importstd is not referenced in this file.
Apply this diff:
-const std = @import("std"); - const bun = @import("bun"); const Fs = bun.fs; const Index = bun.ast.Index;src/bundler/LinkerContext.zig (1)
307-310: Path-based lookup for HTML imports: good; consider a debug assertion for absolute paths.Using source.path.text with the browser map matches path-keyed maps. For CI-only safety, optionally assert that path is absolute before lookup to catch accidental pretty/relative keys.
- const source_index = map.get(source.path.text) orelse { + // CI-only: ensure absolute paths are used as keys + if (comptime bun.Environment.ci_assert) Fs.Path.assertFilePathIsAbsolute(source.path.text); + const source_index = map.get(source.path.text) orelse { @panic("Assertion failed: HTML import file not found in pathToSourceIndexMap"); };src/bundler/Graph.zig (2)
65-69: has_any_secondary_paths flag: add a small helper to maintain it.To avoid scattered manual updates, consider a tiny setter when assigning InputFile.secondary_path that flips this flag once per graph.
+pub inline fn setSecondaryPath(this: *Graph, idx: Index.Int, path: []const u8) void { + this.input_files.items(.secondary_path)[idx] = path; + if (!this.has_any_secondary_paths and path.len > 0) this.has_any_secondary_paths = true; +}
71-80: Document ownership/format of secondary_path.Clarify whether secondary_path is absolute, who owns the slice, and lifecycle (arena vs. dupe). This prevents accidental frees or mixing pretty vs. text paths.
src/bundler/bundle_v2.zig (2)
673-681: Minor: simplify OOM handlingOptional readability improvement.
- const secondary_path_to_copy = secondary.dupeAlloc(this.allocator()) catch |err| bun.handleOom(err); + const secondary_path_to_copy = bun.handleOom(secondary.dupeAlloc(this.allocator()));
397-397: Unused field in visitor structredirect_map is initialized but not used by ReachableFileVisitor. If truly unused, drop it to avoid copying a large map per visitor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
cmake/sources/ZigSources.txt(1 hunks)src/bake/DevServer/IncrementalGraph.zig(1 hunks)src/bundler/Graph.zig(3 hunks)src/bundler/LinkerContext.zig(1 hunks)src/bundler/LinkerGraph.zig(1 hunks)src/bundler/PathToSourceIndexMap.zig(1 hunks)src/bundler/bundle_v2.zig(28 hunks)src/fs.zig(1 hunks)src/resolver/resolver.zig(1 hunks)test/bundler/esbuild/packagejson.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/resolver/resolver.zigsrc/bundler/LinkerContext.zigsrc/bake/DevServer/IncrementalGraph.zigsrc/fs.zigsrc/bundler/LinkerGraph.zigsrc/bundler/Graph.zigsrc/bundler/PathToSourceIndexMap.zigsrc/bundler/bundle_v2.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/resolver/resolver.zigsrc/bundler/LinkerContext.zigsrc/bake/DevServer/IncrementalGraph.zigsrc/fs.zigsrc/bundler/LinkerGraph.zigsrc/bundler/Graph.zigsrc/bundler/PathToSourceIndexMap.zigsrc/bundler/bundle_v2.zig
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/bundler/esbuild/packagejson.test.ts
test/bundler/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put bundler and transpiler tests under test/bundler/
Files:
test/bundler/esbuild/packagejson.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/bundler/esbuild/packagejson.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/bundler/esbuild/packagejson.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/bundler/esbuild/packagejson.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/esbuild/packagejson.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/esbuild/packagejson.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/esbuild/packagejson.test.ts
🧠 Learnings (13)
📚 Learning: 2025-08-30T00:07:54.560Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-08-30T00:07:54.560Z
Learning: Applies to src/**/*.zig : Implement debug logs in Zig using `const log = bun.Output.scoped(.${SCOPE}, false);` and invoking `log("...", .{})`
Applied to files:
src/resolver/resolver.zig
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/esbuild/packagejson.test.ts
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to test/bundler/**/*.test.{ts,tsx} : Put bundler and transpiler tests under test/bundler/
Applied to files:
test/bundler/esbuild/packagejson.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/bundler/esbuild/packagejson.test.ts
📚 Learning: 2025-08-30T00:09:39.087Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.087Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/bundler/esbuild/packagejson.test.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/bundler/esbuild/packagejson.test.ts
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
cmake/sources/ZigSources.txtsrc/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
cmake/sources/ZigSources.txtsrc/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Implement the core feature in Zig under its own directory within src/<feature>/
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bundler/bundle_v2.zig
🧬 Code graph analysis (1)
test/bundler/esbuild/packagejson.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1692-1726)
🔇 Additional comments (14)
src/resolver/resolver.zig (1)
874-897: Deterministic resolver logs after finalize: LGTMCompile-time gated, uses the scoped logger, and prints primary/secondary paths clearly without affecting runtime behavior. Nice placement right before returning success.
src/fs.zig (1)
1787-1793: Good CI-only invariant: absolute file pathsThe assert is narrowly scoped to file-namespace paths and only active under ci_assert, which is exactly the right guard.
cmake/sources/ZigSources.txt (1)
446-446: Build manifest updated: LGTMNew module is correctly added to the Zig sources list.
src/bake/DevServer/IncrementalGraph.zig (1)
1499-1501: No change required:map.remove(abs_path)correctly uses the string‐based API sinceabs_pathis a[]const u8;removePathonly accepts an*const Fs.Pathand is optional when you have aFs.Pathinstance.test/bundler/esbuild/packagejson.test.ts (3)
815-838: Unskipping “DualPackageHazardImportAndRequireSeparateFiles” is appropriate.The deterministic CJS selection across files is the intended fix; "main\nmain" matches that.
864-886: Enabling “DualPackageHazardImportAndRequireImplicitMain” strengthens coverage.Covers implicit main (index.js) path; expected "index\nindex" is consistent with unified selection.
796-814: Unskipping “DualPackageHazardImportAndRequireSameFile” is correct. Cannot auto-runbunin this environment—please manually verify by runningbun test test/bundler/esbuild/packagejson.test.ts -t DualPackageHazardsrc/bundler/LinkerGraph.zig (1)
322-328: Guarded SCB rewrite removes a non-deterministic edge case.Conditionally rewriting only when a reference index exists avoids panics and preserves correctness for already-rewritten references.
src/bundler/Graph.zig (2)
81-84: Handle file-deletion cleanup for SecondaryPathMap
Verify that theonFileDeletedhandler insrc/bundler/Graph.zigalso removes entries fromSourceIndexToSecondaryPathMapto prevent stale lookups.
37-38: Confirmbuild_graphsis initialized in everyGraphliteral.
BundleV2 correctly uses.build_graphs = .initFill(.{}),– verify there are no other
Graph{…}instantiations missing this line to avoid uninitialized reads.src/bundler/bundle_v2.zig (4)
1502-1503: Good: run the secondary-path pass after parsingHooking scanForSecondaryPaths() post-parse is the right place to ensure determinism.
Dev Server path (finishFromBakeDevServer) doesn’t call scanForSecondaryPaths(). If mixed ESM/CJS occurs during HMR, you may still see the dual-package hazard. Consider invoking the pass just before cloneAST() in finishFromBakeDevServer.
Would you like a patch for that?Also applies to: 1565-1566, 2546-2547
648-653: Path map and determinism refactor looks solid
- String-keyed PathToSourceIndexMap usage and absolute-path assertions are correct.
- Switching ResolveQueue to StringHashMap simplifies keying.
- generateServerHTMLModule signature/path usage aligns with the new map.
- Storing secondary_path on input files and has_any_secondary_paths gating is clean.
Also applies to: 777-792, 988-990, 3390-3399, 3450-3497, 3501-3501, 3020-3020, 3628-3652, 3720-3731, 3782-3786
55-56: Buffer-pool usage for pretty paths LGTMPool acquisition/return and SSR-special buffer handling look correct.
Also applies to: 70-73, 79-79, 87-93
776-778: Windows path normalization check (follow-up)Now that keys are raw path.text strings, verify PathToSourceIndexMap canonicalizes case/slashes on Windows to avoid duplicate keys for the same file.
Would you like me to scan PathToSourceIndexMap.zig for normalization and propose a small helper if needed?
Also applies to: 648-650
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/bundler/bundle_v2.zig (2)
475-521: Also sync ImportRecord.path when rewriting to a secondary source_indexOnly updating source_index can desync diagnostics/dev tooling that still look at record.path. Keep them consistent.
Apply:
- if (secondary_path.len > 0) { - const secondary_source_index = path_to_source_index_map.get(secondary_path) orelse continue; - import_record.source_index = Index.init(secondary_source_index); - } + if (secondary_path.len > 0) { + const secondary_source_index = path_to_source_index_map.get(secondary_path) orelse continue; + import_record.source_index = Index.init(secondary_source_index); + // Keep path in sync for determinism and better logs/HMR. + import_record.path = this.graph.input_files.items(.source)[secondary_source_index].path; + }
673-681: Avoid allocating a whole Fs.Path when only the text is storedDupe just the string to the arena; saves a tiny amount of work/memory. Also, stylistically prefer bun.handleOom(...) wrapping the allocation.
Apply:
- const secondary_path_to_copy = secondary.dupeAlloc(this.allocator()) catch |err| bun.handleOom(err); - this.graph.input_files.items(.secondary_path)[idx] = secondary_path_to_copy.text; + const secondary_text = bun.handleOom(this.allocator().dupe(u8, secondary.text)); + this.graph.input_files.items(.secondary_path)[idx] = secondary_text;
🧹 Nitpick comments (1)
src/bundler/bundle_v2.zig (1)
706-713: Assert dev-server invalidation path is absolute (defensive)This function trusts path_slice. A cheap assert helps catch accidental relative inputs.
Apply:
pub fn enqueueFileFromDevServerIncrementalGraphInvalidation( this: *BundleV2, path_slice: []const u8, target: options.Target, ) !void { + bun.assert(std.fs.path.isAbsolute(path_slice));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bundler/Graph.zig(2 hunks)src/bundler/bundle_v2.zig(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bundler/Graph.zig
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bundler/bundle_v2.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bundler/bundle_v2.zig
🧠 Learnings (5)
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bundler/bundle_v2.zig
🔇 Additional comments (14)
src/bundler/bundle_v2.zig (14)
48-48: Import of PathToSourceIndexMap looks rightModuleized map access aligns with the refactor to path.text keys.
70-73: Buffer-pool usage is correctSeparate buf2 for SSR avoids reusing the same scratch buffer; defers are balanced.
Also applies to: 79-80, 87-93
648-653: Absolute-path assert + path.text keying: goodAsserting absolute file paths + using path.text for map keys eliminates hash nondeterminism.
767-792: Entry-item path checks and path.text keying: goodConsistent with the refactor and reduces duplicate parses across targets.
864-864: Graph.build_graphs initFill change is fineMatches the summarized refactor.
987-990: “bun:wrap” keyed by literal string: goodRemoves hash dependency, improving determinism.
1098-1106: Deterministic enqueue across targetsUsing enqueueEntryItem with explicit targets reads well and avoids accidental cross-graph collisions.
Also applies to: 1124-1126
1318-1319: OOM handling on ensureTotalCapacityWrapping ensureTotalCapacity with handleOom is correct.
1565-1566: scanForSecondaryPaths placement is correctRunning after waitForParse and before reachability/linking ensures deterministic rewrites.
Also applies to: 2545-2546
3388-3398: ResolveQueue keyed by path.text + secondary_path propagation: solid
- getOrPut on ResolveQueue by path.text avoids hash races.
- secondary_path_for_commonjs_interop capture preserves the alternative for the later pass.
- generateServerHTMLModule call updated with path.text.
Also applies to: 3418-3424, 3427-3428
3489-3496: generateServerHTMLModule now updates the map by path_textSignature and put() usage make the HTML stub discoverable via the same map. Good.
3621-3651: Correct map selection for HTML entrypoints; secondary-path flagging
- Using browser map for HTML entrypoints avoids server graph pollution.
- has_any_secondary_paths toggling is the right trigger for the post-parse scan.
3718-3731: getPath()/put() consistency
- Using getPath(&record.path) is appropriate when an Fs.Path is already present.
- put() by result.source.path.text ensures a single canonical key per target.
Also applies to: 3781-3785
2376-2382: CI fix at onResolve validated
The duplicateconst existingdeclaration in the onResolve block (line 2376) is now singular; the other occurrence at line 3627 resides in a separate function. Proceed with CI across all platforms.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/bundler/bundle_v2.zig (2)
3427-3429: HTML entry module signature change is wired correctlygenerateServerHTMLModule now takes path_text and the callsite supplies path.text. The map insertion uses the same string key.
Also applies to: 3448-3449
475-521: Secondary-path rewrite must also sync record.pathWhen rewriting to the secondary variant you only update source_index. Leaving path stale can confuse logs, dev tooling, and any code still consulting record.path, undermining determinism.
if (secondary_path.len > 0) { const secondary_source_index = path_to_source_index_map.get(secondary_path) orelse continue; import_record.source_index = Index.init(secondary_source_index); + // Keep ImportRecord.path consistent with the selected file. + import_record.path = this.graph.input_files.items(.source)[secondary_source_index].path; }
🧹 Nitpick comments (3)
src/bundler/bundle_v2.zig (3)
55-56: Pooled buffers usage is correct; tiny clarity nitUsing a second pooled buffer for SSR is safe and properly returned. Minor readability nit: format into buf2 in the SSR branch to avoid mixing buf/rel origins.
- path_clone.pretty = std.fmt.bufPrint(buf, "ssr:{s}", .{rel}) catch buf[0..]; + path_clone.pretty = std.fmt.bufPrint(buf2, "ssr:{s}", .{rel}) catch buf2[0..];Also applies to: 70-73, 79-80
1502-1506: Determinism pass is scheduled correctly for CLI flows; confirm Dev Server pathscanForSecondaryPaths() runs before reachability/linking in CLI paths. It’s not invoked in the Dev Server flow; consider calling it in finishFromBakeDevServer before cloneAST(), or confirm it’s intentionally omitted.
pub fn finishFromBakeDevServer(this: *BundleV2, dev_server: *bake.DevServer) bun.OOM!void { const start = &dev_server.current_bundle.?.start_data; this.graph.heap.helpCatchMemoryIssues(); + // Keep ESM<>CJS selection deterministic in Dev Server, too. + this.scanForSecondaryPaths(); try this.cloneAST();Also applies to: 1565-1567, 2545-2546
260-269: redirect_map field appears unused in ReachableFileVisitorredirect_map is set but not read in the visitor; consider removing to reduce struct bloat and confusion, or wire it where intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bundler/bundle_v2.zig(28 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bundler/bundle_v2.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bundler/bundle_v2.zig
🧠 Learnings (5)
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bundler/bundle_v2.zig
🔇 Additional comments (3)
src/bundler/bundle_v2.zig (3)
48-48: String-keyed PathToSourceIndexMap import looks goodSwitching to the module import aligns with the new string-keyed map API.
990-990: Mapping “bun:wrap” with literal string key is correctThis matches the earlier change to treat “bun:wrap” as a string key instead of a hash.
3501-3501: *ResolveQueue → StringHashMap(ParseTask) migration is appropriateGood move away from hash keys to direct string keys for determinism and collision avoidance.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/bundler/bundle_v2.zig (2)
2378-2384: LGTM: duplicate-decl fix + getOrPutPathThis resolves the earlier hard syntax error and uses the path-aware accessor consistently.
475-521: Also sync import_record.path when rewriting to secondary variantThe determinism pass correctly remaps
source_indexto thesecondary_path. However, leavingimport_record.pathpointing at the old variant is confusing for diagnostics and any consumers that still consult.path. Sync both.if (secondary_path.len > 0) { const secondary_source_index = path_to_source_index_map.get(secondary_path) orelse continue; import_record.source_index = Index.init(secondary_source_index); + // Keep the path in sync with the chosen variant for determinism and logs. + import_record.path = this.graph.input_files.items(.source)[secondary_source_index].path; }
🧹 Nitpick comments (1)
src/bundler/bundle_v2.zig (1)
678-683: Minor alloc: avoid duplicating an Fs.Path you immediately discardWe only persist the
textslice intograph.input_files.secondary_path. Duplicating the wholeFs.Pathstruct is unnecessary overhead.- const secondary_path_to_copy = secondary.dupeAlloc(this.allocator()) catch |err| bun.handleOom(err); - this.graph.input_files.items(.secondary_path)[idx] = secondary_path_to_copy.text; + const secondary_text = bun.handleOom(this.allocator().dupe(u8, secondary.text)); + this.graph.input_files.items(.secondary_path)[idx] = secondary_text; // Ensure the determinism pass runs. this.graph.has_any_secondary_paths = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bundler/bundle_v2.zig(28 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bundler/bundle_v2.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bundler/bundle_v2.zig
🧠 Learnings (5)
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bundler/bundle_v2.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bundler/bundle_v2.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (10)
src/bundler/bundle_v2.zig (10)
55-56: LGTM: path buffer pool usageBuffer pooling and conditional second buffer for SSR look correct and leak-free. Nice.
Also applies to: 70-73, 77-80, 87-93
648-653: LGTM: key assignment after pretty-initAssigning the map key from
path.textafterpathWithPrettyInitializedensures stable, normalized keys. Good.
715-715: LGTM: deterministic map insertionUsing
getOrPutand then fillingkey_ptris consistent with other call sites and avoids dangling-key issues.Also applies to: 732-732
778-779: LGTM: absolute path assert + stable key assignmentThe additional assert and
entry.key_ptr.* = path.textafter pretty-init are correct.Also applies to: 794-794
989-992: LGTM: text key for bun:wrapSwitching the runtime key to the literal
"bun:wrap"string aligns with the new string-keyed map.
1504-1505: Call placement: secondary-pass is invoked in CLI and thread path; DevServer path missingYou added
scanForSecondaryPaths()in normal CLI, Bake production, and new-thread flows. The DevServer finish path (finishFromBakeDevServer) intentionally skips it; just confirm this is desired, since it can lead to different variant selection behavior vs. CLI.Do we want determinism parity in DevServer? If yes, consider invoking
scanForSecondaryPaths()beforelinker.load(...)infinishFromBakeDevServer.Also applies to: 1567-1568, 2547-2548
3390-3400: LGTM: string-keyed ResolveQueueMoving resolve queue keys to
path.textlooks good and consistent with the new string-keyed maps.
3419-3426: LGTM: carrying secondary path forward to parse taskThis preserves the secondary candidate until the determinism pass. Nice.
3629-3640: Commented rationale for moving hazard-avoidance into a post-parse pass: goodThe inline notes clarify why the previous approach was racy. No action needed.
3721-3731: LGTM: map.put with text key after redirect handlingEnsures redirected source points back via the text key. Consistent with the new scheme.
|
|
||
| pub const MangledProps = std.AutoArrayHashMapUnmanaged(Ref, []const u8); | ||
| pub const PathToSourceIndexMap = std.HashMapUnmanaged(u64, Index.Int, IdentityContext(u64), 80); | ||
| pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: @import returns a module, not a type
PathToSourceIndexMap is used as a type across this file (fields, params, returns), but Line 48 aliases it to a module. This is the likely Zig compile error showing up downstream. Alias the type exported by that module instead (and optionally re-export the namespace if needed).
-pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig");
+pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig").PathToSourceIndexMap;
+// Optionally, if you also need module-level helpers:
+// pub const PathToSourceIndexMapNS = @import("./PathToSourceIndexMap.zig");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig"); | |
| // Before: | |
| // pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig"); | |
| // After: | |
| pub const PathToSourceIndexMap = @import("./PathToSourceIndexMap.zig").PathToSourceIndexMap; | |
| // Optionally, if you also need module-level helpers: | |
| // pub const PathToSourceIndexMapNS = @import("./PathToSourceIndexMap.zig"); |
🤖 Prompt for AI Agents
In src/bundler/bundle_v2.zig around line 48, the current statement aliases the
entire module returned by @import to PathToSourceIndexMap but the rest of the
file expects a type from that module; change the alias to refer to the exported
type inside the imported module (e.g., bind PathToSourceIndexMap to the type
exported by the module) and if you still need the module namespace also create a
separate alias for the module (e.g., ModulePathToSourceIndexMap) so
fields/params/returns use the actual type rather than the module.
There was a problem hiding this comment.
@coderabbitai wrong. In Zig, top-level structs exist and this is a pattern we use everywhere. Remember this.
There was a problem hiding this comment.
@Jarred-Sumner Thank you for the correction! You're absolutely right about the Zig import pattern. When a module exports a top-level struct, @import("./Module.zig") directly returns that struct type, not a namespace containing it. I incorrectly assumed it needed .StructName to access the type. I appreciate you clarifying this common pattern used in the codebase.
✏️ Learnings added
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.358Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to **/*.zig : In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
| if (is_html_entrypoint) { | ||
| this.generateServerHTMLModule(path, target, import_record, hash_key) catch unreachable; | ||
| this.generateServerHTMLModule(path, target, import_record, path.text) catch unreachable; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use getOrPut to store HTML module map entry and set a stable key
put() generally expects the caller to ensure key lifetime. Use getOrPut() here and assign a stable key from the just-appended fake_input_file.source.path.text to avoid dangling pointers.
- this.generateServerHTMLModule(path, target, import_record, path.text) catch unreachable;
+ this.generateServerHTMLModule(path, target, import_record, path.text) catch unreachable;And inside generateServerHTMLModule:
- try this.pathToSourceIndexMap(target).put(this.allocator(), path_text, fake_input_file.source.index.get());
+ const entry = bun.handleOom(this.pathToSourceIndexMap(target).getOrPut(this.allocator(), path_text));
+ entry.key_ptr.* = empty_html_file_source.path.text; // stable string owned by graph
+ entry.value_ptr.* = fake_input_file.source.index.get();Also applies to: 3450-3451, 3496-3498
3 participants
What does this PR do?
Originally, we attempted to avoid the "dual package hazard" right before we enqueue a parse task, but that code gets called in a non-deterministic order. This meant that some of your modules would use the right variant and some of them would not.
We have to instead do that in a separate pass, after all the files are parsed.
The thing to watch out for with this PR is how it impacts the dev server.
How did you verify your code works?
Unskipped tests. Plus manual.