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

fix(ESM): Add operators/package.json for ESM support, fixes #3227 #3356

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

jayphelps
Copy link
Member

See #3227

I didn't do anything for @reactivex/rxjs package because I'm hopeful for its elimination (#2916) and I'm not entirely sure how it works lol.

@rxjs-bot
Copy link

rxjs-bot commented Mar 1, 2018

Messages
📖

CJS: 1358.5KB, global: 726.7KB (gzipped: 117.3KB), min: 140.3KB (gzipped: 30.6KB)

Generated by 🚫 dangerJS

typings: './index.d.ts'
typings: './index.d.ts',
module: './_esm5/index.js',
es2015: './_esm2015/index.js'
Copy link
Member Author

Choose a reason for hiding this comment

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

It's hard to keep everything straight, but AFAICT this also makes the whole path-mappings.js thing unnecessary if you're using a newish version of webpack or rollup.

Would be cool to still include instructions (and maybe path-mappings still) for people who are stuck on older build pipelines.

Choose a reason for hiding this comment

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

Yes, if users only import from "rxjs" and "rxjs/operators", importing "rxjs/index" or (more realistically) "rxjs/Rx" would not be mapped.

@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage remained the same at 97.56% when pulling c6d9e8e on jayphelps:esm into 52cdfe8 on ReactiveX:master.

@simonbuchan
Copy link

This says "fixes #3227", but it doesn't handle webpack native tree-shaking? I'd have to check the quality difference between having "sideEffects": false and not in webpack 4, I think with the new uglifyjs in there too it should be able to optimize without it, but it might be slower or something.

@jayphelps
Copy link
Member Author

jayphelps commented Mar 1, 2018

it doesn't handle webpack native tree-shaking?

Can you clarify why it does not? (I tested this btw)

@simonbuchan
Copy link

See the example
It's why I called out "webpack native" tree shaking and mentioned uglify should still work - but presumably the option exists for a reason.

@jayphelps
Copy link
Member Author

jayphelps commented Mar 1, 2018

@simonbuchan oh yeah sideEffects: false might be able to be supported but that's outside the scope of the work I'm doing for this PR and it's being tracked in #3212 not #3227. Perhaps confusing the two? 😄

Copy link
Collaborator

@jasonaden jasonaden left a comment

Choose a reason for hiding this comment

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

This looks correct.

I think we also need a similar package.json file in ajax, testing, and websocket, right?

@jasonaden
Copy link
Collaborator

Agreed on #3212 vs #3227. This PR should close #3227 (as long as a package.json gets added for other secondary entry points such as ajax, websocket, and testing).

#3212 will have another PR shortly (I've got that one).

@simonbuchan
Copy link

@jayphelps Probably! But #3227 reads to me an umbrella issue of "make it just work™".

@jasonaden Is "rxjs/Rx" going away too?

@jayphelps
Copy link
Member Author

@jasonaden

I think we also need a similar package.json file in ajax, testing, and websocket, right?

Ohhh right! Thanks for catching that!

@simonbuchan sorry for the confusion! My primary intention with that ticket was how to prevent two copies of RxJS. 🕺

@jayphelps
Copy link
Member Author

@jasonaden I've updated to include package.json's for ajax/websocket/testing

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants