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

Implement "exports" proposal #72

Open
wants to merge 5 commits into
base: modules-lkgr
from

Conversation

Projects
None yet
6 participants
@guybedford
Copy link
Contributor

commented Jul 2, 2019

This implements the package.json "exports" proposal (https://github.com/jkrems/proposal-pkg-exports/) with the following features:

  • When "exports" are defined for a package ({ "exports": { "./x": "./x.js" }), resolving that package import 'pkg/x', will resolve the exports path (/path/to/pkg/x.js).
  • When a subpath does not match "exports", an error is thrown (see test case).
  • Support for "exports": false to indicate a package has no exports at all.
  • Support for the "." export as an alias of the main.
  • Support for folder exports ({ "./folder/": "./other-folder/" })

Support for "exports" when resolving from CommonJS is not currently implemented.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Amazing work!

I think that we should remove "Support for the "." export as an alias of the main."... this recreates the issue with dual mode.

@devsnek

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

this repo is two months behind master, you may want to re-open this in nodejs/node

@guybedford guybedford force-pushed the guybedford:exports branch from 6657202 to f4df50f Jul 2, 2019

@targos

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This repo is not behind master. It's updated once per day by a Jenkins job

@guybedford guybedford force-pushed the guybedford:exports branch from f4df50f to 3c70334 Jul 2, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

I've also added some documentation and included a folder mapping feature test.

// Loads ./node_modules/es-module-package/src/features/x.js
```

If a package has no exports, setting `"exports": false` can be used instead of

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 2, 2019

Contributor

I think this should just be “any non-nullish value is passed through Object.keys to get the export list; any nullish value disables the feature”, because then “false” is just a convention instead of a magic special value.

This comment has been minimized.

Copy link
@jkrems

jkrems Jul 4, 2019

Contributor

So:

  1. null and undefined: Same as { './': './' }.
  2. Anything else: Normalized as an object (e.g. Object.assign({}, pkgExports)).

Makes sense!

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 4, 2019

Contributor

I’d use { ...pkgExports } but yes, exactly!

This comment has been minimized.

Copy link
@jkrems

jkrems Jul 4, 2019

Contributor

I think guiding people towards a convention here may still have value. How about adding the following sentence:

This is just a convention that works because false, just like {}, has no iterable own properties.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 4, 2019

Contributor

Agreed and that text sounds good to me.

This comment has been minimized.

Copy link
@jkrems

jkrems Jul 4, 2019

Contributor

Done!

Show resolved Hide resolved test/fixtures/pkgexports.mjs Outdated

@jkrems jkrems force-pushed the guybedford:exports branch from 32f4f2b to 29df8a0 Jul 4, 2019

@jkrems jkrems referenced this pull request Jul 4, 2019

Open

Tracking: Unflagging exports in node.js #36

3 of 7 tasks complete
@jkrems

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Support for "." is gone as is the special treatment of false.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Afaict this now has all the pieces for the upstream PR, with CJS support and general polish / edge case hunting being follow-ups while it's experimental.

@jkrems jkrems force-pushed the guybedford:exports branch from c4fdf27 to fd6a107 Jul 4, 2019

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I turned this into a PR against core: nodejs/node#28568

Should we close this PR?

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@jkrems i thought we were going to remove Support for the "." export as an alias of the main.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

@ljharb May be a misunderstanding: What I removed (and there's a test for it not working) is setting the un-postfixed package name export via the exports field. The test to verify that adding a . exports key doesn't work is here: https://github.com/nodejs/node/pull/28568/files#diff-a512c0b2a7207547e7e8e8fb17bd1aa5R9

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

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