Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v18.x] esm: fix --specifier-resolution=node not preserving symlinks #47674

Closed
wants to merge 4 commits into from

Conversation

znewsham
Copy link
Contributor

Fixes #47649

When you combine --experimental-specifier-resolution=node and --preserve-symlinks imports 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 .js is quite a bit of effort - even if that effort will eventually need to be made.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Apr 22, 2023
@richardlau richardlau changed the title ensure --experimental-specifier-resolution=node works with --preserve-symlinks [v18.x] ensure --experimental-specifier-resolution=node works with --preserve-symlinks Apr 22, 2023
@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +313 to +314
resolved = file;
path = fileURLToPath(resolved);
Copy link
Member

Choose a reason for hiding this comment

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

Why are you reassigning resolved? That variable is used again farther down in this function.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Then please add some comments explaining this. The code by itself looks like a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added - but in fairness, the !preserveSymlinks block below also reassigns to resovled.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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.

@aduh95 aduh95 changed the title [v18.x] ensure --experimental-specifier-resolution=node works with --preserve-symlinks [v18.x] esm: fix --specifier-resolution=node not preserving symlinks Apr 23, 2023
znewsham and others added 2 commits April 23, 2023 09:48
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@znewsham
Copy link
Contributor Author

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.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ruyadorno
Copy link
Member

Landed in a00464e

@ruyadorno ruyadorno closed this Aug 17, 2023
ruyadorno pushed a commit that referenced this pull request Aug 17, 2023
Ensure `--experimental-specifier-resolution=node` works when combined
with `--preserve-symlinks`.

PR-URL: #47674
Refs: #47649
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants