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 es2015 entries to the exports declaration #6614

Merged
merged 1 commit into from Oct 6, 2021

Conversation

crisbeto
Copy link
Contributor

@crisbeto crisbeto commented Sep 27, 2021

Based on #6613 (comment). Adds es2015 entries to the exports declaration in the package.json.

@benlesh
Copy link
Member

@benlesh benlesh commented Sep 27, 2021

@IgorMinar @crisbeto .. Is there something we should document, or add to our documentation about how users can leverage this? This seems non-standard.

Looking at the documentation for the exports field should this be "imports" rather than "es2015"? If that, that's fine, I'd just like to know why, and also I'd like to be able to show everyone how to use this (because I think it's less than great that we've defaulted to es5 currently)

@clydin
Copy link
Contributor

@clydin clydin commented Sep 27, 2021

The condition is indeed non-standard. The thought process here was that using the es2015 condition would prevent a potential breaking change for users due to imports now bringing in ES2015 code instead of ES5 code. The scenario would be if a user had a bundler (such as Webpack) configured to use the module top-level field (ESM5) but then the introduction of the exports field in a new version would then cause exports entries to take priority. This would then result in the default (or import if present) condition being used which would change the used code to ES2015 and potentially lead to a broken bundled application.
This setup is not ideal but has two primary goals: minimize exports differences with rxjs 7 and reduce the potential for breaking changes.

However, if it is acceptable for an import statement/expression to bring in ES2015 code with rxjs 6/7 then the default condition could then reference the esm directory instead.

package.json Outdated Show resolved Hide resolved
Based on ReactiveX#6613 (comment). Adds `es2015` entries to the `exports` declaration in the `package.json`.
@IgorMinar
Copy link
Collaborator

@IgorMinar IgorMinar commented Sep 28, 2021

@clydin +1

@benlesh as Charles said changing the default could surprise and break people that already depend on the current behavior.

I do recommend that you update the default and even remove the CJS distribution all together in RxJS v8.

@benlesh
Copy link
Member

@benlesh benlesh commented Sep 29, 2021

RxJS is still used pretty widely in Node. I'm not sure we can remove the CJS version yet.

@IgorMinar
Copy link
Collaborator

@IgorMinar IgorMinar commented Sep 29, 2021

All LTS versions of node support ESM now.

The main consideration for keeping CJS a bit longer is other packages that:

  • depend on RxJS
  • are used in Node, and
  • still want to ship CJS in the future

I suspect that this will extremely rare for any libraries that want to upgrade to RxJS@8 assuming that that's 6+ months away.

@kwonoj
Copy link
Member

@kwonoj kwonoj commented Sep 29, 2021

CJS is not deprecated format while all of LTS node.js started support ESM. Also wide number of tooling ecosystem are not ready for pure esm packages (TypeStrong/ts-node#1007, electron/electron#21457, facebook/jest#9430 for some of examples). I do not think at this rate when we release 8 majority of users are ready to switch from CJS to ESM.

Deprecating CJS in RxJS and supporting tree-shakable ESM (es2015) and native ESM is different story. We can discuss to support better ESM in the future version of RxJS but CJS may need live bit longer.

@IgorMinar
Copy link
Collaborator

@IgorMinar IgorMinar commented Sep 29, 2021

@kwonoj sgtm, the removal of CJS is optional. The only downside of keeping it longer than necessary is the build/test/release complexity and package size.

Let's revisit CJS in the future, when v8 is almost fully baked.

@IgorMinar
Copy link
Collaborator

@IgorMinar IgorMinar commented Sep 30, 2021

@benlesh unlike with PR #6613, we do believe that the issue addressed in this PR needs to be addressed with a bit of urgency because without this change, we can't recommend RxJS v7 to be the default in Angular CLI applications.

Doing so would without this issue fixed, Angular apps (and in fact any web app using RxJS) would default to using es5 as the input into the bundler which results in major size regressions.

In the ideal case default would point to ES2015, but as discussed earlier, we don't recommend doing that now because that would break existing rxjs@7 users, so introducing es2015 instead seems like a reasonable solution, at least until v8 is released. In v8 we recommend revisiting the packaging format and ensuring the at the defaults enable the best optimizations possible.

What can we do to help resolve this issue and enable Angular ecosystem to use RxJS v7 by default rather than as an opt-in? Thank you.

@IgorMinar
Copy link
Collaborator

@IgorMinar IgorMinar commented Oct 6, 2021

This PR fixes #6321 and unfortunately blocks adoption of RxJS v7 by the Angular ecosystem. We'd love for it to be merged and released soon because without this fix any (even non-Angular) application adopting RxJS v7 will see a payload size regression. Thank you for considering.

@benlesh
Copy link
Member

@benlesh benlesh commented Oct 6, 2021

Core team meeting: Agreement that we can merge this in, as it's non-breaking. @IgorMinar mentioned that the Angular team would be willing to document how non-Angular projects can leverage this non-standard config.

Auxiliary discussion: As far as Angular is concerned the latest version of ECMAScript target the better (ES2020 is what they're targeting/using now)

@benlesh benlesh merged commit 268777b into ReactiveX:master Oct 6, 2021
5 checks passed
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.

None yet

5 participants