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

Private name mappings (Imports) #47

Open
jkrems opened this issue Jun 3, 2020 · 12 comments
Open

Private name mappings (Imports) #47

jkrems opened this issue Jun 3, 2020 · 12 comments

Comments

@jkrems
Copy link
Owner

jkrems commented Jun 3, 2020

The proposal currently lists a secondary imports field. This hasn't been implemented anywhere afaik. We should decide what the solution for that use case is.

@jkrems
Copy link
Owner Author

jkrems commented Jun 3, 2020

The suggestions I'm aware of are:

  1. Don't do anything. Packages that want to remap individual files can use self-reference and public entries in their exports fields.
  2. Implement a new field (imports), roughly with the spec in the current README.
  3. Introduce a new sigil in exports that can only be used in self-reference.
  4. Introduce a new condition that only applies in self-reference.

My personal preference is to build on top of self-reference and conditionals and use a new condition to express this.

Imports

"name": "pkg",
"imports": { "#fetch": "./lib/fetch.mjs" }

import '#fetch'; // only visible to code within the package boundary

Exports Sigil

"name": "pkg",
"exports": { "./#fetch": "./lib/fetch.mjs" }

import 'pkg/#fetch'; // only visible to code when using self-reference imports

Exports Condition

"name": "pkg",
"exports": { "./fetch": { "internal": "./lib/fetch.mjs" } }

import 'pkg/fetch'; // only visible to code when using self-reference imports

@guybedford
Copy link
Contributor

guybedford commented Jun 3, 2020

If you look at the example posted in uuidjs/uuid#462 (comment) the first thing one notices is that "exports" is already more verbose than the browser field (and obviously because it carries the ability to hold more information than just browser mappings).

If we use an internal condition name I would be concerned with the level of verbosity in that case, since the internal mapping would then need to additionally compose with the browser mapping and any other mappings, leading to quite deep nesting.

I think it would be beneficial to try and ship something here especially given the resistance @sokra indicated to wanting to implement private mappings on the public API.

I would like to suggest that a variation on the sigil to use a ./# syntax:

{
  "name": "uuid",
  "exports": {
    "./#md5": {
      "browser": "./dist/md5-browser.js",
      "default": "./dist/md5.js"
    },
    "./#rng": {
      "browser": "./dist/rng-browser.js",
      "default": "./dist/rng.js"
    },
    "./#sha1": {
      "browser": "./dist/sha1-browser.js",
      "default": "./dist/sha1.js
    }
  }
}

By retaining the leading ./ we make it clear these are still subpath mappings leaving room for other types of "internal bare specifier" mappings in future as we have so carefully reserved to date as well.

Usage would be via import 'pkg/#internal' so remains fully symmetric with standard exports behaviour.

Note: this variation of the feature is fully valid under current exports - you can ship it now and it will work in Node.js.

That is, the # doesn't change anything else semantically apart from just the private restriction for non internal importers!

@jkrems
Copy link
Owner Author

jkrems commented Jun 3, 2020

Updated the sigil bit to include the ./. The exports sigil would definitely be my 2nd choice and those specifiers are "ugly" enough that they're pretty obviously not an official (public) API.

@guybedford
Copy link
Contributor

The nice thing is that private names are well-enough adopted now for the meaning to be obvious to JS users.

@sokra
Copy link

sokra commented Jun 4, 2020

Using # is not a good idea in my opinion. They have a special meaning in URLs (which these requests are): fragments. We would probably run into problems when trying to translate the exports field into an importMap.

Note: I'm only referring to usage in imports like import​ ​'pkg/#fetch'​;​ or import​ ​'#fetch'​;​. Usage in the exports field would be fine.

@sokra
Copy link

sokra commented Jun 4, 2020

Something like that would also be possible:

"name"​: ​"pkg",​
​"exports"​: ​{​ ​"#/fetch"​: ​"./lib/fetch.mjs"​ ​}​
​"exports"​: ​{​ ​"#./fetch"​: ​"./lib/fetch.mjs"​ ​}​

​import​ ​'pkg/fetch';​ ​// only visible to code when using self-reference imports

But I dislike that as it's not visible from importing site that this is private. I also dislike using self reference import as you have to repeat the package name.

@sokra
Copy link

sokra commented Jun 4, 2020

So to summarize my wishlist would be:

  • clearly visible in import that it's private
  • different name than exports as it's not the public api
  • not using #, ?, + or % as thats reserved in urls
  • support for conditions like exports
  • key in mappings object is equal to request in `import "..."
  • similar syntax as exports
  • not colliding with absolute paths
  • not colliding with possible npm package names
  • support directory mappings like exports

It's only a wishlist, I'm also fine if not everything works.

Here are some options that would fullfill that:

  • import "@/fetch"
  • import "_/fetch"
  • import "[fetch]" and other brackets <>, {}, ()
  • import "$/fetch"
  • import "$fetch"
  • import "*fetch"
  • import "*/fetch" (could be confused with globs)
  • import ":fetch"
  • import "|fetch"
  • import "~/fetch" (often used as reference to app directory)
  • import "~fetch" (often used as reference to app directory)
  • import "^fetch"
  • import "^/fetch"

@jkrems
Copy link
Owner Author

jkrems commented Jun 4, 2020

They have a special meaning in URLs (which these requests are): fragments. We would probably run into problems when trying to translate the exports field into an importMap.

A clarification here: The bare specifiers in import maps aren't URLs and # should be valid in bare specifiers unless import maps chooses to disallow them in the future. So far the direction of import maps has been to not put any restrictions on bare specifiers.

The # would be an issue on the right side (the mapping to a URL) but on the left side I don't expect problems with import maps as things stand.

@guybedford
Copy link
Contributor

Yes I can confirm that # works in Node.js and browsers today with the current import maps and exports field implementations. That actually in many ways makes it a preferable option since it means it is definitely not a URL and hence can only exist in the "virtual space" of the LHS side of the export map.

I also really like the backwards compatibility approach to implementation that came up here.

@sokra
Copy link

sokra commented Jun 5, 2020

If that's the case that's great and I'll take back my critique on that. In this case I prefer the "imports" field.

@coreyfarrell
Copy link

My preference is for the "Exports Sigil" where exports uses LHS ./#fetch and code uses import 'pkg/#fetch'. My complaint with import '#fetch' is that it could create difficulty if using minimatch on specifiers since a string starting with # is treated as a comment. I agree with @guybedford about the internal condition, I think it would be best to avoid adding more nesting to the exports structure.

Can we specifically mention that deep private exports work? For example "./#/": "./" and import 'pkg/#/private/file.js'.

One detail this "Exports Sigil" works today but is not restricted to self-reference.

{
  "name": "pkg",
  "version": "0.1.0",
  "exports": {
    "./#/": "./",
    "./#package.json": "./package.json"
  }
}

This package.json allows the following from inside or outside pkg:

console.log('via "./#/" export', require('pkg/#/package.json'));
console.log('via "./#package.json" export', require('pkg/#package.json'));

This is a good and bad thing. Technically making "./#/": "./" a private export is breaking, but on the other hand it would allow use of internal self-reference from any versions of node.js with self-reference.

@guybedford
Copy link
Contributor

I've put together a PR here in nodejs/node#33780. Feedback welcome.

ctavan added a commit to ctavan/js-multiformats that referenced this issue Jun 24, 2020
First of all: the `pkg.exports` spec defines that object key order is
being used when resolving for the targeted environment, see
uuidjs/uuid#462 (comment)

In order for `webpack@5` to pick up the `browser` overrides they must
come _before_ `import` and `require`, see
https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62 for
preliminary documentation.

To make full use of `pkg.exports` in node as well we can use package
self reference instead of local paths, see the discussion in
jkrems/proposal-pkg-exports#47
ctavan added a commit to ctavan/js-multiformats that referenced this issue Jun 24, 2020
First of all: the `pkg.exports` spec defines that object key order is
being used when resolving for the targeted environment, see
uuidjs/uuid#462 (comment)

In order for `webpack@5` to pick up the `browser` overrides they must
come _before_ `import` and `require`, see
https://gist.github.com/sokra/e032a0f17c1721c71cfced6f14516c62 for
preliminary documentation.

To make full use of `pkg.exports` in node as well we can use package
self reference instead of local paths, see the discussion in
jkrems/proposal-pkg-exports#47

Unfortunately the browser testing tool polendina doesn't play nice with
self reference since it only looks for bare module specifiers in
`node_modules` and `node_modules/polendina/node_modules`, see
https://github.com/rvagg/polendina/blob/master/lib/webpack.config.js#L28-L39

We would need a way to set up an `resolve.alias` for webpack in
polendina to make package self reference work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants