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.
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
Imports proposal - first draft #40
Imports proposal - first draft #40
Changes from 3 commits
617f266
7b16c6d
323d6c0
73597ec
5a53a12
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @GeoffreyBooth i think this exact use case is a reason it’s useful, in that other thread, to have exports override whatever main points to, instead of providing main itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? This suggests having:
{"main": "./foo.mjs", "exports": false}
which would break ifexports
would completely overwritemain
..? It does work if it desugars to merge semantics (Object.assign({ ".": mainValue || [] }, exports)
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that’s a good point, but I’m more thinking conceptually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure I follow; you mean the reason that
exports: false
is useful?A package needs to export something, right?
exports: false
and nomain
would be the package equivalent of unreachable code?I think I agree with @jkrems that merge semantics make the most sense for this, since overriding
main
withexports: false
would have the effect of creating an unusable package in all versions of Node that supportexports
. We’re obviously not dropping support formain
, so ifexports
allows for setting the main as well it would just take precedence.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don’t mean that. I mean that exports has the ability to override whatever main points to. exports false wouldn’t be using that ability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a question of semantics. The docs could simply define
exports: false
as “no exports other than whatever is defined in"main"
.” or something like that. That’s probably what they should say.Because if you think about it, the lack of
"."
inexports
wouldn’t mean that there’s no main export defined for the package. I.e. we’d want to support:Where
require('pkg')
andrequire('pkg/foo')
both work. This would be an example of the merging/Object.assign
approach, rather thanexports
overridingmain
in the sense that this package would not have any main export defined because there’s no"."
inexports
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like you both support the same semantics and the only disagreement is how it should be worded? My vote is for calling it "merge" because as @GeoffreyBooth points out, it's only the
"."
key that actually overrides anything. But I think everyone agrees that we should bring back dot-main in some form (the ability to specifymain
inexports
in a way to wins over the top-levelmain
field).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
"."
should have its own small section in the docs. Something like:Since
"main"
doesn’t define an object it’s a little weird to discuss merging at all. We’re really only talking about the precedence of"main"
versus"."
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we can discuss this main problem at the coming meeting.
I would ideally like to get the same spec here merged so that we can also start to discuss it further though, and wouldn't want these main considerations to block that.
Is there something we can do here to resolve this further? Any note or clarifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any disagreement? It seems like we're just discussing the best way to explain the behavior to users, which is something we can worry about when we get to writing the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
~
for this instead of#
. Parcel and a few other tools already support~/foo
to meanfoo
within the nearest folder with package.json. Supporting~foo
(without the slash) to mean thefoo
named import within the package.json kinda makes sense too.~
always refers to the folder with package.json, and you can either refer to a file or a named import from there. Not sure if Node is interested in the~/
specifier as well, but it would leave the option open for the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think node may be interested in
~/
(or something like it) to mean "this package as exported". Right now there's no good way to unit test the public interface when usingexports
for example. If not~
, we'd need to find another character for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's what I meant.
~/
gets you to the package root, and it's normal resolution after that. So if there is afoo
export, then~/foo
would refer to that as it would normally. And~foo
would refer to an import.~
or~/
by itself could refer tomain
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth opening a dedicated issue to resolve the sigil question? My current thoughts:
We have three kinds of specifiers that people have asked for (implied: that we may or may not want to support):
exports
do not apply to relative specifiers). Examples:✩
(the main/default export),✩/subpath
✩data/emoji.json
,✩fetch
.✩/src/model.mjs
.Of these, only (1) and (3) actually conflict. (2) could share a symbol with either one of them. A concern raised by @guybedford was that if (1) and (2) share a symbol, it may be confusing.
So to me the options are:
~
and~/
to mean "this package as if it was imported by name".~<name>
to be used for custom aliases within the package.~
and ~/` to mean "this package as if it was imported by name".#<name>
for "private names", aliases only visible inside of the package.#/
for "paths relative to the package boundary".For packages that don't use
exports
,~/
and#/
are effectively the same but that would change once they choose to remap subpaths and/or lock themselves down.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference right now would be
~
+#<name>
without support for importing non-public paths relative to the project root. There would still be design space for adding#/
in the future if it becomes truly necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't
#
problematic since it's meaningful to URL parsing? ESM specifiers are URLs, so wouldn't that be considered a hash?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While ESM uses URLs as cache keys in the browser, it has more restrictions for specifiers. The only kinds of specifiers it allows are:
./
,../
, or/
.See: https://html.spec.whatwg.org/#resolve-a-module-specifier
So neither
?
nor#
may start a specifier unless something like an import map is involved. Even though both are valid relative URLs in other contexts like certain HTML attributes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we would promise not to bikeshed yet :P
But if we must then my preference would be to use
~/
for the internal root and#/
or something else for the public interface.Under that logic, perhaps we should use
~name
?But yeah opening a new issue to hash / tilde this out seems to make sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forked this to an issue: #41