Skip to content

Conversation

@nmerget
Copy link
Contributor

@nmerget nmerget commented Dec 12, 2025

closes #102

Copy link
Member

@Benjamin-Dobell Benjamin-Dobell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary fix of package_exports -> package_json looks like a solid bug fix for the default import. However, I believe this would introduce a different bug that would prevent importing of paths from a node module.

@nmerget
Copy link
Contributor Author

nmerget commented Dec 14, 2025

The primary fix of package_exports -> package_json looks like a solid bug fix for the default import. However, I believe this would introduce a different bug that would prevent importing of paths from a node module.

But at the moment the implementation wouldn't work at all.
The resolver is just for resolving from package.json isn't it?
What do you mean with prevent importing of paths from a node module?

@Benjamin-Dobell
Copy link
Member

Benjamin-Dobell commented Dec 14, 2025

Yeah, the implementation is very broken at the moment. It went unnoticed because most packages have exports these days. So your entry-point (.) fix is spot on.

But you ought to be able to do stuff like this too:

import z from 'zod/v4';

Ignore for a moment the fact that zod's package.json has exports. Without exports, this should resolve to ./v4.js (or ./v4/index.js etc.)

If we insert package_exports[dot_slash] = dot_slash;, then this functionality will be preserved. When we go to resolve the import, we'll find the matching ./ and resolve it.

It works like this...

const String file_path = p_module_id.substr(package_name.length() + 1);

if (check_package_file_path(absolute_package_path, file_path, o_source_info))

That file_path becomes p_module_id in check_package_file_path, and it's passed through into:

String DefaultModuleResolver::resolve_package_export(const Dictionary& p_exports, const String& p_condition, const String& p_module_id)
{
    String lookup_key = p_module_id.is_empty() || p_module_id == "."
        ? "."
        : "./" + p_module_id;

Because p_module_id is not empty, we attach a ./ prefix as the lookup key. So in the example, it'd become ./v4.

Then we look through the (generated in this case) package exports for the longest matching prefix. . matches, but ./ matches too and is longer, so we pick that. It just maps to ./ (module root), which is what we're after (by default).

@Benjamin-Dobell Benjamin-Dobell merged commit 0a6aa43 into main Dec 14, 2025
38 checks passed
@Benjamin-Dobell Benjamin-Dobell deleted the fix-node-module-resolver branch December 14, 2025 23:24
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.

godotjs 4.4 is unable to import mobx, and underlying issues obscured

3 participants