[v18.x] esm: fix --specifier-resolution=node not preserving symlinks#47674
[v18.x] esm: fix --specifier-resolution=node not preserving symlinks#47674znewsham wants to merge 4 commits intonodejs:v18.x-stagingfrom
--specifier-resolution=node not preserving symlinks#47674Conversation
|
Review requested:
|
GeoffreyBooth
left a comment
There was a problem hiding this comment.
This has already used up more time than I think it deserves. I won’t block this if others feel it’s worth the maintenance burden, but I think this is misplaced effort.
| resolved = file; | ||
| path = fileURLToPath(resolved); |
There was a problem hiding this comment.
Why are you reassigning resolved? That variable is used again farther down in this function.
| resolved = file; | |
| path = fileURLToPath(resolved); | |
| path = fileURLToPath(file); |
I’m still wary of this change. As it modifies variables that are used outside of the if (getOptionValue('--experimental-specifier-resolution') === 'node') scope, it could have unintended consequences for anyone passing that flag. Those unintended consequences would be further bugs imposed upon the modules team to fix.
There was a problem hiding this comment.
The bug is that resolved is not reassigned when it should be. If preseveSymlinks is passed in the function just returns resolved and never (meaningfully) looks at path
There was a problem hiding this comment.
Then please add some comments explaining this. The code by itself looks like a mistake.
There was a problem hiding this comment.
Added - but in fairness, the !preserveSymlinks block below also reassigns to resovled.
aduh95
left a comment
There was a problem hiding this comment.
Thanks for sending this PR, it looks good to me (modulo the comment nits) and rather safe. At first, I was under the impression that it might produce conflicts when we'll try to backport other ESM changes to v18.x, but since the diff is limited to part of the codebase that have been removed from main, that should be fine :)
I would still encourage you to move away from --specifier-resolution ASAP, either by picking a loader package on npm that does the same thing or by creating your own, I would be happy to help if you need assistance on this.
--specifier-resolution=node not preserving symlinks
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
Thanks @aduh95 - if you could point me to some of the ESM loader packages that would be great - I did try to roll my own, and it mostly works, but it's not something I'd be comfortable using in production at all really, even once it stops being experimental. An even better approach would be to just remove all our imports that use ambiguous paths - but that will be an ongoing effort. |
|
Landed in a00464e |
Fixes #47649
When you combine
--experimental-specifier-resolution=nodeand--preserve-symlinksimports that were valid in node 14 fail.While I recognise that this behaviour is experimental, and indeed removed in node 19, swapping to a loader for the simple case of appending
.jsis quite a bit of effort - even if that effort will eventually need to be made.