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

Should libraries that depend on rxjs use path-mappings/tree shaking? #3227

Closed
jayphelps opened this issue Jan 15, 2018 · 20 comments · Fixed by Xenira/Shashki#66
Closed

Should libraries that depend on rxjs use path-mappings/tree shaking? #3227

jayphelps opened this issue Jan 15, 2018 · 20 comments · Fixed by Xenira/Shashki#66

Comments

@jayphelps
Copy link
Member

jayphelps commented Jan 15, 2018

👋 This might at first glance seem like an obvious question, but let me explain:

By default in v6 if you import anything it's going to resolve the files from the project root. e.g. import { Observable } from 'rxjs' is going to come from node_modules/rxjs/Rx.js which re-exports it from node_modules/rxjs/internal/Observable.js.

If a library uses the path-mappings and tree shaking they would use custom resolve overrides. So in that case, Observable would come from node_modules/rxjs/_esm5/index.js which re-exports it from node_modules/rxjs/_esm5/internal/Observable.js. Note the fact that this is inside rxjs/_esm5.

Then let's say that someone imports my library (e.g. https://redux-observable.js.org) which had done this, but the consumer does not set up their own webpack resolve overrides so they'll get Observable from node_modules/rxjs/internal/Observable.js and now we have two copies of rxjs in the same project. Observable1 instanceof Observable2 === false. If they use pipeable operators they probably won't immediately run into errors, if ever, but their bundle size will be silently larger and might later get cases where Observable1 instanceof Observable2 === false, which is quite the hard thing to diagnose for most.

If the library doesn't use path-mappings, the same problem exists in reverse; if the library consumer do use it, they'll again get their own copy of RxJS.


This was brought up previously when we had rxjs-es (because a redux-observable user hit it) and discussed including the esm5 versions, but I don't recall a solution ever being found.

Yes, this sucks. AFAIK the only real solution would be have the esm5 version in its own package (or make it the default package and put the other stuff in another). Interestingly, if that was done I think people wouldn't need to have path-mappings and instead could just alias "rxjs": require.resolve('rxjs-esm5') or whatever it would be called. This could still give you two copies but to me feels like it reduces the likelihood as long as libs set their peerDep correctly.

🔥

@benlesh
Copy link
Member

benlesh commented Jan 15, 2018

Ahh. I hate bundlers.

Not sure what to do here.

cc @IgorMinar @TheLarkInn

@IgorMinar
Copy link
Contributor

path-mappings will not be needed in v6. so things will work correctly.

there is however a good chance of some problems if you will need to use v6+legacy-path-support package. we'll need to investigate this.

but overall, I'm not very concerned.

@jayphelps
Copy link
Member Author

path-mappings will not be needed in v6. so things will work correctly.

v6 of Angular or v6 of RxJS? If RxJS, can you elaborate why it won't need it? What if the bundler I use doesn't support es modules? won't I get a different version of RxJS if I import a library that did use them?

@jayphelps
Copy link
Member Author

jayphelps commented Feb 12, 2018

I did a bunch of research and experimenting around this issue and it seems like there is indeed a solution: the "module" package.json field convention supported by bundlers like webpack. It is used to point to source that uses ESM instead of CJS, and at least in the latest webpack is only used if all dependencies agree on its usage for a particular mutual dependency.

e.g. if App depends on library X and RxJS, and X itself also depends on RxJS, webpack will only use the ESM build provided by "module" if and only if X also supports "module". If it does not, it basically deopts into just using the standard "main" CJS because otherwise it would have to pull in two copies of RxJS (one from "main" for X, and one from "module" for App) it actually still uses the ESM version but basically emulates the CJS interface for the dependencies that need it, which prevents tree shaking still. (thanks to @cilice for the correction!) This is a bit confusing, so the tl;dr is just that you won't have two copies in those deopt cases, but you won't get tree shaking either. Acceptable IMO

Here's an example of this behavior for testing: https://github.com/jayphelps/webpack-module-test You can modify the dependencies in various way to confirm how webpack behaves.

There's still the possibility that someone will set up their app and get tree shaking for RxJS then later install some other dependency that deopts them without knowing, losing tree shaking silently. This sucks...but not sure there's anything we can do about this other than potentially changing webpack to warn when this happens (if using a flag or something). We can also document this fact in the install instructions, though it's a tough problem to explain to the average person who isn't as aware of how tree shaking/ESM works vs CJS.

@benlesh
Copy link
Member

benlesh commented Feb 22, 2018

@jayphelps @IgorMinar ... would it help this issue at all if we only had one export site? If we did what I originally wanted to do and had everything export from rxjs?

@benlesh
Copy link
Member

benlesh commented Feb 22, 2018

cc @jasonaden

@jayphelps
Copy link
Member Author

@benlesh yep.

@everyone I've done more experiments and am now convinced that we need to export everything from the root "rxjs" instead of "rxjs/operators" and use the "modules" convention to point to the ESM builds. Otherwise people will have to have custom alias stuff to get operators from ESM and also have issues with two copies of rxjs (CJS and ESM) if they and the libraries they depend on don't knowingly prevent it.

I think if we do it that way, everything will "just work".

@IgorMinar you mentioned you don't think this is an issue, can you elaborate? Was angular going to provide custom aliases to the ESM builds for "rxjs/operators"?

@jasonaden
Copy link
Collaborator

@jayphelps I believe this is the issue we need to resolve: #3212

Basically, if we have side-effect-free packages and Webpack 4, we don't need to provide any aliases.

I can provide more detail if needed. Running out real quick, but I'll check back on this issue in about 90 minutes.

@jayphelps
Copy link
Member Author

jayphelps commented Feb 22, 2018

@jasonaden Interesting! Could you elaborate on how it works without people still aliasing the paths to the ESM builds? This demo from the related angular issue still uses the path aliases

I was under the (perhaps incorrect) impression that sideEffects: false only worked with ESM?

@jasonaden
Copy link
Collaborator

So the complete change would also include adding the module key to the package.json file pointing to the ESM distribution. main to the CJS version, module to ESM5, es2015 to ESM2015, and typings to the typings root.

It's basically following the Angular Package Format. The format will be updated again for Angular 6 (in combination with Webpack 4 and pure imports) to be clear that libraries don't need to (and in fact shouldn't) flatten their ESM distributions. This was previously required as otherwise Webpack's module overhead added bloat, and dead code elimination was limited. But with Webpack 4 it's no longer a problem.

@jayphelps
Copy link
Member Author

jayphelps commented Feb 22, 2018

@jasonaden Sorry to belabor, wouldn't that require rxjs to switch to flat, non-nested imports? i.e. no "rxjs/operators", as discussed above? I thought "module" only works with flat entry.

@jasonaden
Copy link
Collaborator

Ah, right. No, it won't require that. We will to distribute a multiple entry point package.

Take a look at Angular's common package. The package.json you see in the root is the one you hit when importing from @angular/common. But if you import from @angular/common/http, you'll get the package.json here. And similarly, you import from @angular/common/http/testing and you'll get the one [here].

So it will be similar with the rx distribution where there will be multiple package.json files that get distributed, but all will come with a single npm install rxjs.

@jayphelps
Copy link
Member Author

jayphelps commented Feb 22, 2018

@jasonaden wow! I had never thought of using nested package.json and that webpack would then correctly use "module". That's clever AF! 🍻

So far I'm sold on that (or also down with just moving to flat single level imports all from "rxjs")

@jasonaden
Copy link
Collaborator

Yeah, that took a bit of playing with it and a bunch of testing before we were sure it would work. But it's pretty cool! Probably deserves a blog post somewhere at some point... it's super useful for libraries.

@TheLarkInn
Copy link

webpack also just supports adding a different packageField file as well in case you didn't want the extra package.js but this is likely the most node and standard way to do it.

@simonbuchan
Copy link

Or much simpler, move to using a sibling .mjs file and everything magically works (on webpack 4, before you have to add .mjs to resolve.extensions yourself)

There is also https://webpack.js.org/configuration/resolve/#resolve-aliasfields - you could point browser at _esm5 by default, and document that you can add another field to that config to get _esm2015 (or higher).

// package.js
{
  // ...,
  "main": "index.js", // node
  "browser": { // bundlers: webpack + browserify
    "./index.js": "./_esm5/index.js",
    "./operators.js": "./_esm5/operators.js"
  },
  "es2015": {
    "./index.js": "./_esm2015/index.js",
    "./operators.js": "./_esm2015/operators.js"
  },
  // ...
}

@simonbuchan
Copy link

Also, with "sideEffects": false, this means optimal zero-config webpack. They just added support for explicitly listing some files as having side-effects if that was blocking.

@jayphelps
Copy link
Member Author

#3356

jayphelps added a commit to jayphelps/rxjs that referenced this issue Mar 1, 2018
jayphelps added a commit to jayphelps/rxjs that referenced this issue Mar 1, 2018
@benlesh benlesh closed this as completed in 725dcb4 Mar 2, 2018
@erickj
Copy link

erickj commented Mar 9, 2018

Is there an expected date for the next 6.x alpha release so that the changes for pkg.module are available in the npm repos?

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