module: resolve self-references #29327
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkrems
added
the
esm
|
I have to dig up the PR for introducing a scope / namespace... but |
|
@MylesBorins Unfortunately basically anything but |
|
It isn't a valid path on Windows though. With it being a tier 1 platform do
we have to worry about breaking it?
…On Mon, Aug 26, 2019, 6:11 PM Jan Olaf Krems ***@***.***> wrote:
@MylesBorins <https://github.com/MylesBorins> Unfortunately basically
anything but \0 is a valid Unix directory name. So namespaces etc. won't
remove the need for making it resolve last for backwards compat. But it
could help make it more obvious that it's "special"!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29327?email_source=notifications&email_token=AADZYV32XQHZW7LTXBBAYO3QGRIHLA5CNFSM4IPWVHE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5F22IQ#issuecomment-525053218>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADZYV3IZODLSGGRX5UJKF3QGRIHLANCNFSM4IPWVHEQ>
.
|
|
This dovetails with the concept of package scope we’re introducing in ESM, as Of course I assume you plan on making this work in ESM too; we should make sure it doesn’t conflict with potentially eventually supporting browser import maps, if they have any plans for |
Isn’t it the opposite? On Windows we know that |
We may have to check that trying to stat on Windows won’t throw (so we actually get to our logic). The specifier not working as-is across platforms would be a good thing because it reduces the risk that somebody used it already for other purposes. |
|
cc @nodejs/modules |
|
On *nix: date > '~' ; cat < '~'Or on win32: date > '~' ; type < '~'However, the fact that it is valid doesn't necessarily mean it is portable. Lack of portability in particular on the myriad of package registries means that it is probably safe I would think, a quick search of require on gzemnid naive findingsThat comes out to 92 modules in the public registry using this pattern.From github it is harder to perform the search since |
devsnek
previously requested changes
Aug 27, 2019
|
Would it be more reasonable to add a separate api for this rather than relying on figuring out a new specifier? |
|
@Fishrock123 that would be much more confusing and complex for the massive ecosystem that handles resolution; and it wouldn’t work as nicely with ESM. It’s far cleaner to do it via specifier imo. |
|
@devsnek tilde is the most commonly used alias in my experience (babel/webpack/TS) for this exact feature, package root. Do you have an alternative suggestion? |
|
|
|
Since |
|
Wouldn't the namespace approach make it different enough from home directory? |
|
@MylesBorins Ah, gotcha. Right, if the issue is "~/ could be confused with the home directory", then we may be able to compromise on |
|
My issue with the tilde is more of the fact that it's the first character in the string, which for the last 30 years has meant "expand to $HOME" in unix, and however long powershell has existed. If we're trying to make paths less confusing, I don't think using tilde at the start is the best approach. Perhaps |
|
My 2c is over at nodejs/modules#306 (comment)
|
|
Requiring the package name defeats one of the benefits of having the alias, which is “not repeating the package name”. |
|
@Fishrock123 TC39 is irrelevant here; there’s no concept of a “package” and the spec intentionally avoids imposing any restrictions on specifier contents. |
|
my idea would be that
|
|
by "named-export" you mean, any specifier that's reachable from the package root, filtered through |
|
FWIW, Parcel and other tools already implement tilde resolution as it is being proposed here. https://parceljs.org/module_resolution.html I don't think anyone would expect |
I agree with this. Non-portable code is uncommon, and we shouldn’t assume that that’s the default or the user expectation. The only use case I can think of is the Node equivalent of shell scripts, but even those would probably usually use relative references. I think there’s an advantage to reusing the |
|
Userland code can do whatever it wants, people opt into it. We don't plan to let this be overridden and I know at least three people I've talked to who would want avoid using tilde as an expansion. |
targos
approved these changes
Oct 24, 2019
|
Landed in 71bcd05 |
jkrems
added a commit
that referenced
this pull request
Oct 24, 2019
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
| 4. LOAD_NODE_MODULES(X, dirname(Y)) | ||
| 5. THROW "not found" | ||
| 5. LOAD_NODE_MODULES(X, dirname(Y)) | ||
| 4. LOAD_SELF_REFERENCE(X, dirname(Y)) |
There was a problem hiding this comment.
Are these entries misnumbered?
There was a problem hiding this comment.
They are! Let me create a follow-up to fix that.
jkrems
added a commit
to jkrems/node
that referenced
this pull request
Oct 25, 2019
targos
pushed a commit
that referenced
this pull request
Oct 28, 2019
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos
pushed a commit
that referenced
this pull request
Nov 8, 2019
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos
added
the
semver-minor
2 tasks
targos
pushed a commit
that referenced
this pull request
Jan 8, 2020
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos
pushed a commit
that referenced
this pull request
Jan 8, 2020
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 6, 2020
Adds the ability to `import` or `require` a package from within its
own source code. This allows tests and examples to be written using
the package name, making them easier to reuse by consumers of the
package.
Assuming the `name` field in `package.json` is set to `my-pkg`, its
test could use `require('my-pkg')` or `import 'my-pkg'` even if
there's no `node_modules/my-pkg` while testing the package itself.
An important difference between this and relative specifiers like
`require('../')` is that self-references use the public interface
of the package as defined in the `exports` field while relative
specifiers don't.
This behavior is guarded by a new experimental flag
(`--experimental-resolve-self`).
PR-URL: #29327
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See nodejs/modules#306
requestisn't a relative path and wasn't resolved successfully using the existing algorithm.package.json(the "package scope"), bail if none can be found.requestmatches thenamefield ofpackage.json.requestagainst the package as if requiring from the outside.package.json#name) to be used.package.json#name, checknode_modulesfirst.The unflagging issue should cover:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes