--preserve-symlinks and --experimental-specifier-resolution=node fails to find file without extension

[znewsham]

Version

16.20

Platform

Linux ed7d7cb37749 5.19.0-38-generic #39~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Mar 17 21:16:15 UTC 2 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

create two JS files:

// index.js
import "./test"

// test.js
// whatever

and a package.json

{ "type": "module" }

run:

node  --preserve-symlinks --experimental-specifier-resolution=node index.js

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

Always.

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

It should be able to find the correct JS file without an extension

What do you see instead?

[Error: ENOENT: no such file or directory, open '/path/to/test'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/path/to/test'
}

Additional information

This worked correctly in node 14. I know this flag is going away in node 19, but I don't really want to have to implement an entire loader just to get extensionless loading working the same as it did in 14 - particularly irksome because the loader API changed between 14 and 16, so to support both versions of node I need to support both these experimental features.

Adding the .js extension isn't practical.

The error is caused by this code in internal/modules/esm/resolve

  if (!preserveSymlinks) {
    const real = realpathSync(path, {
      [internalFS.realpathCacheKey]: realpathCache
    });
    const { search, hash } = resolved;
    resolved =
        pathToFileURL(real + (StringPrototypeEndsWith(path, sep) ? '/' : ''));
    resolved.search = search;
    resolved.hash = hash;
  }

  return resolved;

there should probably be an else:

else { 
  resolved = path
}

[aduh95]

--experimental-specifier-resolution=node is experimental and has been removed as of Node.js 19.0.0. Consider switching to using custom loaders, as the runtime warning asks you to do.

[znewsham]

I'm a little surprised this won't be handled, while the flag is removed in node 19, the combination is valid in node 16 and 18 (both still active under lts) and the functionality is unarguably broken AND was working in node 14. The option that replaces it is also experimental the API of which changed dramatically between 14 and 16, so it's not like I'd be replacing something experimental with something stable. It's also not a trivial task to implement a loader for this trivial case, i did it as I can't wait for this option to be fixed but the implementation isn't complete and required copying chunks of code out if the internal resolve function. It also appears to be a 1 line change. Would you accept a pr for it?

…

On Sat, Apr 22, 2023, 6:20 AM Antoine du Hamel ***@***.***> wrote: Closed #47649 <#47649> as not planned. — Reply to this email directly, view it on GitHub <#47649 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBKQC6QXMPMVUBUEHVDTLLXCOWHVANCNFSM6AAAAAAXF55NGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[aduh95]

The only way to find out pull be to open a PR against v18.x-staging and see if it goes through, but I wouldn’t be too optimistic about it, typically no changes can land on an LTS line without a “maturation period” on a Current release line. The final decision is up to @nodejs/lts, maybe they can say more on the process and the success chances of such PR.

[richardlau]

@aduh95 It's not impossible for such a change to land in an active LTS line -- it would depend on how complicated/risky the change looks and we'd probably want some sort of sign off/review/ok from the relevant team (@nodejs/loaders ?). If the change would only be behind an experimental flag that would count positively towards it.

Changing Node.js 16 (in maintenance) is almost certainly out ruled out.

[GeoffreyBooth]

The loaders team is very resource constrained and it’s not a worthwhile use of their time to devote any effort into bugfixing a removed API.

I would oppose landing something in the 18.x line only, as 20.x is very new and few users have migrated to it yet; and 16.x is in maintenance mode, leaving 18.x as our most widely used active LTS line.

[znewsham]

Thanks for your comments all - I opened a PR, the change is very simple and I believe safe. If it doesn't get merged, so be it

#47674

[aduh95]

@znewsham I know of https://github.com/nodejs/loaders-test/tree/main/commonjs-extension-resolution-loader that aims at doing this specifically, not sure it's been published on npm though.

[znewsham]

That's amazing, thanks! At the very least I can see how they work, and I might swap to using that rather than my own! It's also a lot simpler than mine 🤔 I guess the resolve package deals with most of the complexity