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

Add exports within package.json to enable scoped package loading. #6192

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

samccone
Copy link
Contributor

@samccone samccone commented Mar 29, 2021

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


Test case

import {Observable} from 'rxjs';
import {scan} from 'rxjs/operators';

console.log(Observable, scan);
{
  "type": "module",
  "dependencies": {
    "rxjs": "^7.0.0-beta.13" // With the above modification
  }
}

package.json Outdated Show resolved Hide resolved
Copy link
Member

@benlesh benlesh left a 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.

package.json Outdated Show resolved Hide resolved
@benlesh
Copy link
Member

benlesh commented Mar 29, 2021

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.

@benlesh
Copy link
Member

benlesh commented Mar 29, 2021

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.

@samccone
Copy link
Contributor Author

As for a test @benlesh, the test is actually quite simple!

test_file.mjs

import {Observable} from 'rxjs';
import * as o from 'rxjs/operators';
import * as a from 'rxjs/ajax';
import * as f from 'rxjs/fetch';
import * as t from 'rxjs/testing';
import * as w from 'rxjs/webSocket';

console.log(Observable, o, a, f, t, w);

In node 14,15 run node test_file.mjs against the built package. Do you have any similar tests to this currently?

@samccone samccone requested a review from benlesh March 29, 2021 21:32
@benlesh benlesh requested a review from cartant March 29, 2021 22:47
@benlesh
Copy link
Member

benlesh commented Mar 29, 2021

Do you have any similar tests to this currently?

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
@samccone
Copy link
Contributor Author

ok @benlesh test case added and new github workflow config has been modified to run the test.

Copy link
Collaborator

@cartant cartant left a 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. 😢

@MylesBorins
Copy link

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

@benlesh
Copy link
Member

benlesh commented Mar 30, 2021

is whether we really want to be downleveling our ES modules to ES5. It's 2021 and distributing ES5 makes me cry. 😢

Same... I want to get rid of those. Let's add it the the agenda.

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

@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?

@benlesh
Copy link
Member

benlesh commented Mar 30, 2021

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

@benlesh benlesh merged commit 33a9f06 into ReactiveX:master Mar 30, 2021
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 23, 2021
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.
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 23, 2021
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.
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 23, 2021
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.
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 27, 2021
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.
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 27, 2021
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.
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

Successfully merging this pull request may close these issues.

4 participants