nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.6k stars 29.07k forks source link

Node does not determine that a file is an ES module when the package.json is in a long path #54304

Open BalaM314 opened 1 month ago

BalaM314 commented 1 month ago

Version

22.6.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

No response

What steps will reproduce the bug?

Create a .js file with the following code:

console.log("Hello world!");
export {};

and a package.json file with the following code:

{
    "type": "module"
}

in a directory with path length greater than 260 characters

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Node should look at the closest package.json, see "type": "module", determine that the file is an ES module, and run it

What do you see instead?

Node is unable to read the package.json, defaults to CommonJS, and fails at export {}

Additional information

Showcase

RedYetiDev commented 1 month ago
# repro.sh
prefix=$(printf 'a%.0s' {1..128})

# Remove directory if it exists, then recreate it
rm -rf "$prefix" && mkdir -p "$prefix/$prefix"

# Create index.js and package.json in the nested directory
echo "export 'Loaded!';" > "$prefix/$prefix/index.js"
echo '{"type":"module"}' > "$prefix/$prefix/package.json"

# Run the Node.js script to import the module and log the output
node -p "import('./$prefix/$prefix/index.js').then(console.log)"
$ bash repro.sh
Promise {
  <rejected> SyntaxError: Unexpected token 'export'
      at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
      at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:166:18)
      at callTranslator (node:internal/modules/esm/loader:436:14)
      at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:442:30)
      at async ModuleJob._link (node:internal/modules/esm/module_job:106:19)
}
file:///<...>/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/index.js:1
export 'Loaded!';
^^^^^^

SyntaxError: Unexpected token 'export'
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:166:18)
    at callTranslator (node:internal/modules/esm/loader:436:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:442:30)
    at async ModuleJob._link (node:internal/modules/esm/module_job:106:19)

Node.js v22.6.0
anonrig commented 1 month ago

We probably miss ToNamespacedPath call somewhere

BalaM314 commented 1 month ago

First time contributing to node, or any C++ project

I've tried to fix this, and here's what I found: Problem is likely in src/node_modules.cc at the function BindingData::GetPackageScopeConfig There isn't a comment explaining what this function does, but I think it's this:

C++ function that accepts a JS string (URL of directory, should be dir\ or dir\file, not dir) Throws a JS error if the URL is invalid Throws a JS error if URL\package.json is invalid If the path ends with node_modules, or the search fails, return a JS string with the last attempt for path to package json (even if nonexistent) Find file_path of the package.json as a std::string Returns GetPackageJSON(file_path)->Serialize() as a js value

The function uses the URL parser to go to the parent path for some reason, and there is a comment saying it should not do that Also, the function doesn't call ToNamespacedPath, which @anonrig said was the likely cause of this issue

I rewrote the function to use std::filesystem::path instead of the URL parser and it seems to work The function ToNamespacedPath requires a BufferValue, which requires a JS string with the path in it, but we don't have that because this function takes a URL as the js argument, not a path I found some comments saying that the function ToNamespacedPath should also support being called with a string, should I do that?