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 to package.json on 6.x branch #6613

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Contributor

This is along the same lines as #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 crisbeto marked this pull request as ready for review September 23, 2021 21:10
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm! thank you @crisbeto! 🚀 ❤️

package.json Show resolved Hide resolved
@crisbeto
Copy link
Contributor Author

Feedback has been addressed.

@benlesh
Copy link
Member

benlesh commented Sep 27, 2021

The changes look okay, but I'm still double checking what/how this might break folks. It's not readily apparent to me that someone's build won't go belly-up if they're running ^6, given that this is going to effect bundlers in ways I'm not quite familiar with.

@benlesh
Copy link
Member

benlesh commented Sep 27, 2021

NOTE: The ts@latest CI failure can be ignored. It's untyped try/catch blocks we still need to update in the 6.x branch.

crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 27, 2021
Based on ReactiveX#6613 (comment). Adds `es2015` entries to the `exports` declaration in the `package.json`.
crisbeto added a commit to crisbeto/rxjs that referenced this pull request Sep 27, 2021
Based on ReactiveX#6613 (comment). Adds `es2015` entries to the `exports` declaration in the `package.json`.
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.
@benlesh
Copy link
Member

benlesh commented Sep 30, 2021

After some discussion:

  • @cartant pointed out that this change will probably break users of rxjs-compat, which 6.x needs to support.
  • @trxcllnt noted that this will possibly break node users, because they may be requiring things in differently than bundled apps, and node won't allow you to require anything that isn't explicitly in the "exports" map. @kwonoj pointed out that we've defined /internal/* so that's probably not an issue.
  • Several unanswered questions about the es2015 conditional and who/what uses it.

@crisbeto @IgorMinar Can you address any of these concerns?

IIRC, when this was added by @samccone, there was some discussion about whether or not it should go into the 6.x branch as well, and there was general agreement that it would constitute a breaking change so we opted to not do that. However I'm not able to find the conversation. It might be that since we added the special handling for internal/*, it's not going to break anyone. There just seems to be some uncertainty about that right now. rxjs-compat adds a new angle on that I hadn't considered, and I'm glad that @cartant pointed it out.

As it stands, I think we'll want to wait until the next RxJS core team meeting so we can reach consensus on this. Right now it seems like the majority of RxJS core team members are leaning towards "unsure" or "no, because it's a breaking change".

It may be that we just need to decide what amount of breakage is acceptable as well. (That's the sort of decision people make when they fix bugs for even patch releases, after all) If it's very low risk, it's probably okay to release this as a minor, IMO — but again, I think the team needs to reach consensus before we act

@benlesh benlesh added 6.x Issues and PRs for version 6.x AGENDA ITEM Flagged for discussion at core team meetings labels Sep 30, 2021
@IgorMinar
Copy link
Contributor

I've discussed this with the team, and @clydin is going to respond with more details, but we understand the hesitation and support you doing the right thing for everyone — whatever that ends up being.

@clydin
Copy link
Contributor

clydin commented Oct 1, 2021

For usage with the Angular CLI, we are now able to apply a temporary workaround for rxjs 6 usage by internally enabling a Webpack option that disables ESM compliant behavior for a package or module. While this allows Webpack-based bundling to succeed, it is not an ideal solution as it is Webpack specific and does not necessarily apply to other bundlers nor Node.js itself. For the Angular CLI, accumulating package specific workarounds is also, unfortunately, not an ideal or sustainable long-term solution. For these reasons, if possible, it would be preferred that the package was made ESM compliant via this or a similar PR. However, we do understand there is a balance that must be achieved when considering the potential for breaking changes.

In regards to mitigating breaking changes, there is also the option to add support for full deep importing of the package via a final subpath pattern ("./*": “./*”). This effectively disables the encapsulation normally provided by the exports field.

@benlesh
Copy link
Member

benlesh commented Oct 6, 2021

Core Team Meeting: Angular has a workaround for this. So this isn't as important.

benlesh pushed a commit that referenced this pull request Oct 6, 2021
#6614)

Based on #6613 (comment). Adds `es2015` entries to the `exports` declaration in the `package.json`.
@benlesh
Copy link
Member

benlesh commented Jan 4, 2022

This seems unnecessary at this point, so I'm going to close it.

@benlesh benlesh closed this Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.x Issues and PRs for version 6.x AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants