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

Imports proposal - first draft #40

Merged
merged 5 commits into from Aug 15, 2019

Conversation

@guybedford
Copy link
Contributor

commented Aug 12, 2019

This provides details on the corresponding package "imports" proposal as the second field as part of this proposal, extending the similar types of mappings we have for "exports" into cases that can work for internal aliasing as well.

Notes have been provided where details are still to be determined, in particular whether or what custom symbol will be used for disambiguation in mappings.

Help fleshing out more edge cases in the examples here would be great.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Show resolved Hide resolved README.md
README.md Outdated

`"exports"` works in concert with the `package.json` `"type": "module"` signifier that a package can be imported as ESM by Node - `"exports"` by itself does not signify that a package should be treated as ESM.
Both features can be supported in both CommonJS and ES modules.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

🎉

README.md Outdated
@@ -80,7 +78,9 @@ Rough outline of a possible resolution algorithm:

In the future, the algorithm might be adjusted to align with work done in the [import maps proposal](https://github.com/domenic/import-maps).

### Usage
For packages that only have a main and no exports, `"exports": false` can be used as a shorthand for `"exports": {}` providing an encapsulated package.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 12, 2019

Owner

Can you elaborate? This suggests having: {"main": "./foo.mjs", "exports": false} which would break if exports would completely overwrite main..? It does work if it desugars to merge semantics (Object.assign({ ".": mainValue || [] }, exports)).

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

that’s a good point, but I’m more thinking conceptually.

This comment has been minimized.

Copy link
@GeoffreyBooth

GeoffreyBooth Aug 12, 2019

Collaborator

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.

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 no main 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 with exports: false would have the effect of creating an unusable package in all versions of Node that support exports. We’re obviously not dropping support for main, so if exports allows for setting the main as well it would just take precedence.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@GeoffreyBooth

GeoffreyBooth Aug 12, 2019

Collaborator

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 "." in exports wouldn’t mean that there’s no main export defined for the package. I.e. we’d want to support:

"main": "./main.js",
"exports": { "./foo": "./foo/index.js" }

Where require('pkg') and require('pkg/foo') both work. This would be an example of the merging/Object.assign approach, rather than exports overriding main in the sense that this package would not have any main export defined because there’s no "." in exports.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 12, 2019

Owner

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 specify main in exports in a way to wins over the top-level main field).

This comment has been minimized.

Copy link
@GeoffreyBooth

GeoffreyBooth Aug 12, 2019

Collaborator

Perhaps "." should have its own small section in the docs. Something like:

"." can specify the root entry point for a package, i.e. the file loaded for require('pkg') or import 'pkg'. Historically this has been defined by the package.json "main" field, and "main" will continue to be supported. If "main" is defined and "." is not, the value in "main" will determine the package root entry point; if both "main" and "." are defined, the latter takes precedence. If "main" is defined and exports is set to false, the value in "main" is the only export available for the package.

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 ".".

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 13, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@GeoffreyBooth

GeoffreyBooth Aug 13, 2019

Collaborator

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.

README.md Outdated
@@ -120,6 +120,69 @@ import utc from '@momentjs/moment/timezones/utc/'; // Note trailing slash
// Error: folders cannot be imported (there is no index.* magic)
```

### 2. Imports Field

> **To avoid conflict with `node_modules` packages, the current proposal prefixes all imports with `#name`, so that the fact that an alias is being imported is clear. Whether this restriction is maintained in the final proposal, or what symbol is used is still TBD.**

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

we don’t have to bikeshed here, but i prefer ~

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 13, 2019

Author Contributor

Definitely open to that.

README.md Outdated

As with package exports, mappings are mapped relative to the package base, and keys that end in slashes can map to folder roots.

The resolution algorithms remain the same except `"exports"` provide the added feature that they can also map into third-party packages that would be looked up in node_modules, including to subpaths that would be in turn resolved through `"exports"`. There is no risk of circular resolution here, since `"exports"` themselves only ever resolve to direct internal paths and can't in turn map to aliases.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 12, 2019

Contributor

what if package A has an imports map of “x” to “B/y”, and package B has an imports map of “A/x”?

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 13, 2019

Author Contributor

Do you mean something like:

A/main.js import '#x'
B/y.js: import 'A/main.js'

where #x -> B/y and B/y -> B/y.js is an exports map in B?

That would construct a cycle between the modules, yes, but the cycle point was about the resolver itself getting caught in a resolution cycle.

For example, if we supported package exports mapping into external packages then you could set up a cycle where B/x -> A/x -> B/x sort of thing, where the resolver cannot complete the resolve operation.

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
@jkrems

jkrems approved these changes Aug 12, 2019

Copy link
Owner

left a comment

Happy to land this as-is (maybe with the minor typo fix) and go from there!

guybedford and others added some commits Aug 13, 2019

Update README.md
Co-Authored-By: Jan Olaf Krems <jan.krems@gmail.com>
@bmeck

This comment has been minimized.

Copy link

commented Aug 13, 2019

I'm curious how this interacts with https://github.com/nodejs/node/blob/master/doc/api/policy.md#dependency-redirection which are controlling at the app level and not package level.

@jkrems

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2019

My gut feeling would be "policy applies to the raw specifier, preventing any resolution including imports". But it may be worth clarifying it in the proposal.

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Seems like we may be good to merge here then?

Further feedback welcome.


Imports provide the ability to remap bare specifiers within packages before they hit the node_modules resolution process.

The current proposal prefixes all imports with `#` to provide a clear signal that it's a _symbolic specifier_ and also to prevent packages that use imports from working in any environment (runtime, bundler) that isn't aware of imports.

This comment has been minimized.

Copy link
@devongovett

devongovett Aug 14, 2019

I like ~ for this instead of #. Parcel and a few other tools already support ~/foo to mean foo within the nearest folder with package.json. Supporting ~foo (without the slash) to mean the foo 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.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 14, 2019

Owner

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 using exports for example. If not ~, we'd need to find another character for this.

This comment has been minimized.

Copy link
@devongovett

devongovett Aug 14, 2019

I guess that's what I meant. ~/ gets you to the package root, and it's normal resolution after that. So if there is a foo export, then ~/foo would refer to that as it would normally. And ~foo would refer to an import. ~ or ~/ by itself could refer to main.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 14, 2019

Owner

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):

  1. Getting the public interface of the package. This will allow people using exports to actually unit test their packages (since exports do not apply to relative specifiers). Examples: (the main/default export), ✩/subpath
  2. Adding custom aliases that are only valid inside of the package boundary. Examples: ✩data/emoji.json, ✩fetch.
  3. Accessing arbitrary paths relative to the package boundary. Examples: ✩/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:

  1. One sigil, no (3):
    • Use ~ and ~/ to mean "this package as if it was imported by name".
    • Allow ~<name> to be used for custom aliases within the package.
  2. Two sigils, optional support for (3):
    • Use ~ and ~/` to mean "this package as if it was imported by name".
    • Use #<name> for "private names", aliases only visible inside of the package.
    • (optional) Use #/ 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.

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 14, 2019

Owner

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.

This comment has been minimized.

Copy link
@devongovett

devongovett Aug 14, 2019

Isn't # problematic since it's meaningful to URL parsing? ESM specifiers are URLs, so wouldn't that be considered a hash?

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 14, 2019

Owner

While ESM uses URLs as cache keys in the browser, it has more restrictions for specifiers. The only kinds of specifiers it allows are:

  1. Relative specifiers starting with ./, ../, or /.
  2. An absolute URL, including protocol.

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.

This comment has been minimized.

Copy link
@guybedford

guybedford Aug 15, 2019

Author Contributor

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!

This comment has been minimized.

Copy link
@jkrems

jkrems Aug 15, 2019

Owner

Forked this to an issue: #41

@jkrems

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2019

(Resolved conflicts using UI after merging the validations PR. I think I picked the right lines.)

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Looks good, shall we merge and move the symbol discussions to their own threads then?

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

(I can also get behind changing to ~ if people want now too)

@jkrems jkrems referenced this pull request Aug 15, 2019

Open

The Great Sigil Lottery #41

@jkrems

This comment has been minimized.

Copy link
Owner

commented Aug 15, 2019

Let's merge and discuss the sigil question separately (#41).

@jkrems jkrems merged commit 0d746d0 into jkrems:master Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.