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

module: disable package self resolution when name contains a colon #37290

Closed
wants to merge 3 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 9, 2021

There is a discrepancy between package self resolution in CJS and ESM contexts. This commit fixes this by forbidding self-resolution of packages whose names contain a colon.

Currently, ESM understands specifier with a column as a URL scheme (E.G.: file:package references package under the scheme file:), while CJS interpret the specifier as a path, so the colon is just another valid character (I.E.: file:package references a package named file:package).

I think this should be considered as a bug fix and backported as far as we can, this is especially important considering it affects stuff like require('node:fs') which should take a different meaning soon (see #37246).

Note that while this is technically a breaking change, I'm confident it should not affect the ecosystem as npm already requires a name that doesn't contain colons (https://docs.npmjs.com/creating-a-package-json-file#required-name-and-version-fields), plus package self-resolution usually affects dev-env only.

/cc @nodejs/modules

There is a discrepancy between package self resolution in CJS and ESM
contexts. This commit fixes this by forbidding self-resolution of
packages whose names contain a colon.

q
@ljharb
Copy link
Member

ljharb commented Feb 9, 2021

Does this PR affect any other CJS resolution for oaths with a colon, or only the new self-resolution feature?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 9, 2021

Does this PR affect any other CJS resolution for oaths with a colon, or only the new self-resolution feature?

Only CJS self-resolve, if the specifier contains a ::

if (selfResolved) {
  if (StringPrototypeIncludes(request, ':')) {
    throw new ERR_INVALID_MODULE_SPECIFIER(
      request, 'is not a valid package name', parentPath);
  }

@ljharb
Copy link
Member

ljharb commented Feb 9, 2021

It seems really strange that require('foo:bar') would work from outside a package, but not from within it - what's the benefit of only partially limiting use of the colon?

npm's requirements aren't the whole of the ecosystem, and any linked dependency (including in a workspace/monorepo) might never be published, and might use a colon, and might be relying on this feature.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 9, 2021

npm's requirements aren't the whole of the ecosystem, and any linked dependency (including in a workspace/monorepo) might never be published, and might use a colon, and might be relying on this feature.

That's a good point, and I guess it's difficult to measure…

It seems really strange that require('foo:bar') would work from outside a package, but not from within it

On that point, on Windows, I believe it currently works only from within, but not from outside.

@richardlau richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2021
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

it seems like we're faced with either an inconsistency between windows and non-windows, or an inconsistency between general CJS requireability, and CJS self-resolution. Personally the former seems more palatable. What do others think?

@@ -613,7 +613,8 @@ Then any module _in that package_ can reference an export in the package itself:
import { something } from 'a-package'; // Imports "something" from ./main.mjs.
```

Self-referencing is available only if `package.json` has [`"exports"`][], and
Self-referencing is available only if `package.json` has a [`"name"`][] that
does not contain the character `:`, and a [`"exports"`][] field, and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
does not contain the character `:`, and a [`"exports"`][] field, and
does not contain the character `:`, and an [`"exports"`][] field, and

@guybedford
Copy link
Contributor

guybedford commented Feb 9, 2021

The ESM loader currently includes the following validation:

If packageName starts with "." or contains "\" or "%", then
Throw an Invalid Module Specifier error.

It might be nice to treat ":" as just another one of the valid characters being checked here for consistency in the ESM loader as well for consistency, even though it doesn't have the same problem as it supports URLs directly.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Mar 9, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2021

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2021

because using a bot to close issues is hostile

@aduh95
Copy link
Contributor Author

aduh95 commented May 15, 2021

Closing as stalled.

@aduh95 aduh95 closed this May 15, 2021
@aduh95 aduh95 deleted the self-resolve-colon branch May 15, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants