fix: prefer search paths over parent-relative node_modules walk#224
Open
benpalevsky wants to merge 1 commit into
Open
fix: prefer search paths over parent-relative node_modules walk#224benpalevsky wants to merge 1 commit into
benpalevsky wants to merge 1 commit into
Conversation
For bare module specifiers, try `find_module_resolver(normalized_id)`
(which iterates the configured search paths) before the parent-relative
node_modules chain walk. The walk remains as a fallback for non-hoisted
pnpm/yarn layouts where a curated `.godot/GodotJS/<dep>.js` bundle isn't
present.
Without this, `require("immer")` resolved via the Bun symlink chain to
`node_modules/immer/dist/cjs/index.js`, whose first real statement is
`if (process.env.NODE_ENV === 'production')` — undefined in our runtime.
The pre-bundled `.godot/GodotJS/immer.js` (which Bun processed to strip
`process` references) was reachable via search path 1
(`res://.godot/GodotJS`) but never won the resolution race.
Reconciled on top of a8e2567 ("fix: default (index.js) module resolution
infinite recursion"). That fix lives entirely in
bridge/jsb_module_resolver.cpp (check_package_file_path, the empty
p_module_id guard), reached identically via find_module_resolver from
both the search-path lookup and the node_modules walk in
bridge/jsb_environment.cpp. Reordering the two find_module_resolver calls
does not bypass or weaken the recursion guard: any exports-less package
root lookup still returns false and lets check_absolute_file_path fall
through to the explicit module_id/index.js probe.
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
1 participant
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.
Problem
When your code imports a bare package name — like
require("immer")— GodotJS has to figure out which file that means. It has two ways to look:res://.godot/GodotJS/), where clean, pre-bundled copies of dependencies live.node_moduleswalk up the folder tree, the way Node.js resolves packages. With Bun/pnpm symlinks, this lands on the raw npm package.The old code tried the
node_moduleswalk first. Sorequire("immer")found the raw npm filenode_modules/immer/dist/cjs/index.js, whose first real statement checksprocess.env.NODE_ENV. There is noprocessin the GodotJS runtime, so it broke. The clean pre-bundled copy at.godot/GodotJS/immer.js(which Bun had already processed to strip out theprocessreferences) was sitting right there on a search path, but it never got the chance to win.What this changes
Flip the order: try the search paths first, and only fall back to the parent-relative
node_moduleswalk if the search paths don't have the module. Note thatres://node_modulesis itself the last configured search path, so a hoisted rootnode_modulespackage still resolves when no curated bundle exists; the parent-relative walk now only matters for nested (non-hoisted)node_modulesdirectories, which consequently no longer shadow a same-named module sitting on a search path.Relationship to the recent recursion fix
This is built on top of upstream's recent fix for "default (index.js) module resolution infinite recursion." That fix lives in a different file (the module resolver) and is reached the same way no matter which lookup runs first, so reordering the two lookups does not weaken it. Verified directly: the recursion guard is untouched, and a package directory with no
exports/mainfield still falls through to itsindex.jsexactly as before.How to verify
Rebuild, then run a project headless (
godot --headless --path <project>) where a script doesrequire("<dep>")and bothres://.godot/GodotJS/<dep>.jsandnode_modules/<dep>exist; confirm the curated bundle loads (noprocess is not defined). Also confirm a directory import of anexports-less package still resolves to itsindex.jswithout hanging.A changeset is included.