module: resolve self-references#29327
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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
hybrist added the esm 
Contributor
MylesBorins commented Aug 26, 2019
I have to dig up the PR for introducing a scope / namespace... but |
ContributorAuthor
hybrist commented Aug 26, 2019
@MylesBorins Unfortunately basically anything but |
Contributor
MylesBorins commented Aug 26, 2019 via email
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> . |
Member
GeoffreyBooth commented Aug 26, 2019
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 |
Member
GeoffreyBooth commented Aug 26, 2019
Isn’t it the opposite? On Windows we know that |
ContributorAuthor
hybrist commented Aug 26, 2019
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. |
Member
ljharb commented Aug 27, 2019
cc @nodejs/modules |
Member
bmeck commented Aug 27, 2019
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
Member
devsnek left a comment
devsnek left a comment There was a problem hiding this comment.
if this isn't pointing to os.homedir() i don't think tilde should be used. Just too surprising/confusing.
Contributor
Fishrock123 commented Aug 27, 2019
Would it be more reasonable to add a separate api for this rather than relying on figuring out a new specifier? |
Member
ljharb commented Aug 27, 2019
@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. |
Member
ljharb commented Aug 27, 2019
@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? |
Member
devsnek commented Aug 27, 2019
|
ContributorAuthor
hybrist commented Aug 27, 2019
Since |
Contributor
MylesBorins commented Aug 27, 2019
Wouldn't the namespace approach make it different enough from home directory? |
ContributorAuthor
hybrist commented Aug 27, 2019
@MylesBorins Ah, gotcha. Right, if the issue is "~/ could be confused with the home directory", then we may be able to compromise on |
Member
devsnek commented Aug 27, 2019
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 |
Contributor
My 2c is over at nodejs/modules#306 (comment)
|
Member
ljharb commented Aug 27, 2019
Requiring the package name defeats one of the benefits of having the alias, which is “not repeating the package name”. |
Member
ljharb commented Aug 27, 2019
@Fishrock123 TC39 is irrelevant here; there’s no concept of a “package” and the spec intentionally avoids imposing any restrictions on specifier contents. |
Contributor
MylesBorins commented Aug 27, 2019
my idea would be that
|
Member
ljharb commented Aug 27, 2019
by "named-export" you mean, any specifier that's reachable from the package root, filtered through |
Contributor
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 |
Member
GeoffreyBooth commented Aug 27, 2019
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 |
Member
devsnek commented Aug 27, 2019
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. |
ContributorAuthor
hybrist commented Aug 27, 2019
@devsnek Asking explicitly: Would Myles' proposal change things (using |
Member
@jkrems my critera above was "what happens if i put it in bash". For example on my computer However, thinking about this further, I'm curious about whether this feature in general goes against our "browser compatibility" goals. |
targos approved these changes Oct 24, 2019
Collaborator
nodejs-github-bot commented Oct 24, 2019
ContributorAuthor
hybrist commented Oct 24, 2019
Landed in 71bcd05 |
hybrist 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
| 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)) |
Contributor
There was a problem hiding this comment.
Are these entries misnumbered?
ContributorAuthor
There was a problem hiding this comment.
They are! Let me create a follow-up to fix that.
hybrist added a commit to hybrist/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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>hybrist added a commit that referenced this pull request Oct 29, 2019
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Nov 5, 2019
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>targos pushed a commit that referenced this pull request Nov 8, 2019
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>targos pushed a commit that referenced this pull request Jan 8, 2020
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>targos pushed a commit that referenced this pull request Jan 8, 2020
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refs: #29327 PR-URL: #30117 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
20 participants
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