-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add exports within package.json to enable scoped package loading. #6192
Conversation
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 totally sure of the implications here. But I can see that we're missing a few spots.
I'm really glad you're looking into this, @samccone. Also, I guess we'd need to be running some sort of simple test that shows the problem that this is fixing, and that it's working now. Thank you, @MylesBorins for helping to review this, BTW. |
Also, @samccone and @MylesBorins, since you're both Googlers, you might want to try this change to the package.json inside of google3 to see if it breaks anything. I don't think it does, because Google builds from the TypeScript source, but I have no idea what their internal tooling is doing with package.json files during any of what they do. |
As for a test @benlesh, the test is actually quite simple! test_file.mjs
In node 14,15 run |
We have some import tests that @kwonoj set up, but I'm not sure they cover this. That test is so simple that it seems like just running that would be fine, IMO. |
…oading. In order for module resolution to work with .mjs or package: module code we need to utilize the conditional export feature of node. > The current specifier resolution does not support all default behavior of the CommonJS loader. One of the behavior differences is automatic resolution of file extensions and the ability to import directories that have an index file. https://nodejs.org/api/esm.html#esm_customizing_esm_specifier_resolution_algorithm https://nodejs.org/api/packages.html#packages_conditional_exports This directly enables rxjs to work with mjs code and commonjs code since mjs does not support loading bare folder paths. This is a fix to: sveltejs/kit#612 and is directly related to the conversation in this issue within node core nodejs/node#27408
ok @benlesh test case added and new github workflow config has been modified to run the test. |
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.
LGTM.
Widespread support for exports
in package.json
is something I've been looking forward to for a long time, so this is particularly encouraging.
One thing we might want to look at - not necessarily in this PR, but it's something I think we should discuss before v7 is released - is whether we really want to be downleveling our ES modules to ES5. It's 2021 and distributing ES5 makes me cry. 😢
@cartant moving to native ESM will be an even bigger lift as Node.js doesn't support File Extension Resolution or Folder Resolution by default... So every internal import will need to be updated in the source (or modified in transpiled output). There are creative ways to continue to use extensionless imports internally without requiring any flags (Specifically Subpath Pattern), but they do require using a non-relative import specifier and I don't believe that TS supports this yet. |
Same... I want to get rid of those. Let's add it the the agenda.
@MylesBorins Can you summarize what we need to do to be the best stewards of the community here? What should we ideally be outputting? How can we test it? |
Actually, @MylesBorins, if you have the time, I'd love it if you started a new issue for this with some checkboxes of what we need to have, and how we could test it? No worries if you don't have the time. We're all volunteers here. I want to merge this, and try to get the conversation somewhere it makes sense. <3 |
This is along the same lines as ReactiveX#6192, but for the 6.x branch. On Angular we're exploring changing our packaging to be based around ES modules and rxjs is currently a blocker, because it doesn't specify the `exports` field on the `package.json` in the 6.x branch like it does in 7.x. These changes add an identical configuration.
This is along the same lines as ReactiveX#6192, but for the 6.x branch. On Angular we're exploring changing our packaging to be based around ES modules and rxjs is currently a blocker, because it doesn't specify the `exports` field on the `package.json` in the 6.x branch like it does in 7.x. These changes add an identical configuration.
This is along the same lines as ReactiveX#6192, but for the 6.x branch. On Angular we're exploring changing our packaging to be based around ES modules and rxjs is currently a blocker, because it doesn't specify the `exports` field on the `package.json` in the 6.x branch like it does in 7.x. These changes add an identical configuration.
This is along the same lines as ReactiveX#6192, but for the 6.x branch. On Angular we're exploring changing our packaging to be based around ES modules and rxjs is currently a blocker, because it doesn't specify the `exports` field on the `package.json` in the 6.x branch like it does in 7.x. These changes add an identical configuration.
This is along the same lines as ReactiveX#6192, but for the 6.x branch. On Angular we're exploring changing our packaging to be based around ES modules and rxjs is currently a blocker, because it doesn't specify the `exports` field on the `package.json` in the 6.x branch like it does in 7.x. These changes add an identical configuration.
In order for module resolution to work with .mjs or package: module code
we need to utilize the conditional export feature of node.
https://nodejs.org/api/esm.html#esm_customizing_esm_specifier_resolution_algorithm
https://nodejs.org/api/packages.html#packages_conditional_exports
This directly enables rxjs to work with mjs code and commonjs code since
mjs does not support loading bare folder paths.
This is a fix to:
sveltejs/kit#612
and is directly related to the conversation in this issue within node
core nodejs/node#27408
Test case