refactor: node native esm

[calebboyd]

Description:

This PR changes the package to a "type": "module" package. Adding an exports entry to the package.json which going forward will do the work of the generate-alias helper

• Appends .js to each import
• Fixes up some double quotes.
• Moves .make-helpers to a tool/make-helpers
• Keeps tools as commonjs
• Keeps dist/spec dist/cjs and dist/src as commonjs (aligning with typescript emit)

Related issue (if exists):

#6030 #4416 #2858 #3858

General beginning for deno support via compatibility layer #5350 #5486

Breaking Changes

• When a user is on node > 12.7 They will no longer be able to use deep import paths, and can only use what is specified in package.json#exports.

  • Eg. if a user is accessing require('rxjs/dist/cjs') Node will throw
    a ERR_PACKAGE_PATH_NOT_EXPORTED error.

• A user on early EOLd node 13 using --experimental-modules will be importing a commonjs module as esm since conditional entries are not supported. node 13 compat dropped in Add exports within package.json to enable scoped package loading. #6192

[kwonoj]

Thanks for tackling this - we have few internal attempt and failed to make things work to satisfy for some conditions.

Please ensure this should pass at least

• source code itself, intellisense and others should able to navigate to proper source code
• existing cjs / esm export should work (since we can't make breaking changes for v7). If we were to deprecate existing esm2015, this should be major breaking and goes for v8 or beyond
• import test suite (https://github.com/ReactiveX/rxjs/tree/master/integration/import) is very important to ensure it's not breaking. Currently it's cjs only, but with esm it should have corresponding esm suite and both should able to pass test. if you open up existing cjs suite, it checks export via require - same goes for esm, it should check export via esm and both should pass
• unit test (mocha) should able to run cjs / esm as separate suite to validate changes.
• sourcemap should work for cjs / esm both, for node.js and browers (no regressions)

[kwonoj]

I'm marking as blocked for now since this is in-progress.

[kwonoj]

Personally, I'm very hesitant to postfix .js extension to all of our source codes, but if that makes all things work may be feasible. Still prefer there's some postprocess to handle those instead of all src keeps it manually. /cc @cartant @benlesh

[ncjamieson]

Personally, I'm very hesitant to postfix .js extension to all of our source codes, but if that makes all things work may be feasible.

FWIW, my understanding is that this is the correct and recommended approach as far as TypeScript is concerned.

[kwonoj]

FWIW, my understanding is that this is the correct and recommended approach as far as TypeScript is concerned.

Back in the days I recall there's some problem around intellisense or other ts service things, is it now working?

[ncjamieson]

... around intellisense or other ts service things

TBH, I'd be hesitant to say that the entire TS ecosystem will be working with it, despite it being the recommended approach. I'll have a look at my notes, this evening, for some links to 'official' TS statements. At work, ATM, hence the non-cartant GitHub account.

[kwonoj]

TBH, I'd be hesitant to say that the entire TS ecosystem will be working with it, despite it being the recommended approach. I'll have a look at my notes, this evening, for some links to 'official' TS statements.

I peeked few ts issues back in time and afaik official recommendation is bit behind of supporting things. (we have to double confirm but) if recommended change requires some churns around navigating code, or intellisense or vice versa I'd say it's not something we can easily adapt.

[kwonoj]

Emits package "type": "commonjs" to cjs dist folder (might need to do this in all non esm distros too)

mind elaborate this? we have single package.json for published pkg (i.e https://unpkg.com/browse/rxjs@7.0.0-beta.9/package.json) and don't have per-dist specific.

[kwonoj]

I would like to recommend to split commits between other changes to appending .js extensions, otherwise reviewers are not possible to go through changes easily.

[calebboyd]

mind elaborate this?

The top level package is type: module If the runtime is supporting that it will follow the exports map accordingly for esm.

Otherwise it will revert to exports[*].require, (or main and aliases folder structure if sufficiently old).
To invoke the cjs loader within a esm package, we have to include a package.json#type=commonjs.

https://github.com/ReactiveX/rxjs/pull/6043/files#diff-f9b4e2ee34781b16f6c4ae4801b021ad200776de33c06d9a1f96a2443ff2c471R32

[kwonoj]

Otherwise it will revert to exports[*].require, (or main and aliases folder structure if sufficiently old).
To invoke the cjs loader within a esm package, we have to include a package.json#type=commonjs.

oh, I mean node.js docs / spec reference I can read about.

[calebboyd]

oh, I mean node.js docs / spec reference I can read about.

This is a good place to start https://nodejs.org/dist/latest-v15.x/docs/api/packages.html#packages_modules_packages

[kwonoj]

This is a good place to start https://nodejs.org/dist/latest-v15.x/docs/api/packages.html#packages_modules_packages

I am aware of those docs. I think I missed part you specifically mentioned about cjs interop and would like to read those part.

[calebboyd]

Gotcha, Maybe this: (I'm using conditional exports to support dual mode) https://nodejs.org/dist/latest-v15.x/docs/api/packages.html#packages_dual_commonjs_es_module_packages

[cartant]

Regarding the .js extension business, this is the comment which I have linked in my ES module-related notes, but there is a more-recent comment that was made before that issue was closed.

[calebboyd]

I would like to recommend to split commits between other changes to appending .js extensions, otherwise reviewers are not possible to go through changes easily.

Split commits added! This is ready for review.
If there are any other tests or things you'd like to have added or clarified please let me know.

[kwonoj]

where's package.json for esm?

[calebboyd]

I'm forcing esm with the .mjs extension in these tests.

[kwonoj]

I think I'm bit confused. Purpose of this integration test is trying to mimc actual user's package.json with various config, so we can't enforce something since we can't ask user to adjust package.json. That's reason there was cjs/package.json, and I expect esm have same package.json have type:module to test esm import when user decides to go with native esm. So in total, we expect at least 3 tests - cjs without any type (for node.js doesn't support type property) / cjs with type:cjs / esm with type:module.

User can choose either use mjs, or decide type:module and treat all js as esm.

[kwonoj]

Mind elaborate how current test covers above cases?

[calebboyd]

My thought was that by invoking cjs and mjs explicitly with file extensions, I'm exacting the codepaths in node that load the code with the correct loader. Whether that loader option is invoked via a package.json or a type field is a node implementation detail that may expand in the future. IMHO adding a separate package.json is just another way node gives you to choose a loader, but I don't think it needs to be a focus of these tests.

Right now the assertions being made are ensuring that the module exports are equivalent when run via cjs and esm.

[kwonoj]

IMHO adding a separate package.json is just another way node gives you to chose a loader but I don't think it needs to be a focus of these tests.

as long as your test can represent real user's package.json usecase to test, it should be ok. But I'm bit hard to understand if it's covered. So current way can we verify flows like below?

1. user uses old / latest node.js, package.json doesn't have type field and want to use cjs
2. user uses latest node.js, package.json have type field to cjs, want to use cjs
3. user uses latest node.js package.json have type field to esm, want to use esm
4. (need to be added) webpack / other bundler picks up esm correctly

Whether that loader option is invoked via a package.json or a type field is a node implementation detail that may expand in the future

This isn't about node.js internal, As stated this test try to mimic current real world package.json structures as user load their module in various different way. Regardless of how node.js treats, we should support existing ergonomics user uses.

[calebboyd]

For sure, For the flows you outlined - the answers are declared in the package.json

1. main field is discovered -> dist/cjs is loaded (node 11 in the tests is doing this)
2. exports field is discovered -> import ? -> dist/esm : dist/cjs (node 12+) Edit: - added node 13 array fallbacks to cjs for early 13 versions
3. same as 2
4. Webpack implements resolution logic based on these fields as well

I don't know if there is value to adding more tests here since the added test now validates that both esm and cjs can be requested and contain a consistent API.

[calebboyd]

This is highlighting the new feature that is added in this PR -- currently these lines of code are not possible in 7.0.0-beta.10

[benlesh]

Core Team Meeting Notes: We've decided we want to do this at some point, but version 7 isn't the right time to do so. We'd like to let this ecosystem simmer for a bit and address this maybe in version 8.

[calebboyd]

Closing for now then, thanks for the update