ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.84k stars 3.01k forks source link

Rename main entry /Rx.js to /index.js #1888

Closed jayphelps closed 7 years ago

jayphelps commented 8 years ago

Many in the community have started importing all of RxJS by directing referencing the entry file "rxjs/Rx"

import Rx from 'rxjs/Rx';

Instead of just importing it from the module root "rxjs" and letting their resolver decide where to find it (usually via package.json main field)

import Rx from 'rxjs';

This is weird to me and is I've noticed is confusing to some people who believe there is a difference between the two other than the first one is more brittle, relying on the the exact name of the entry file.

Is there a particular reason we named it Rx.js instead of the usual index.js? I realize changing this now will certainly break people who rely on this, so if anyone is worried I'm not opposed to a single deprecation release with a console.warn().

Also--do any TypeScript pros know if there is any between the two from a typing perspective?

anaisbetts commented 8 years ago

This is because this is what the documentation tells you to do (or at least did at some point), in the section about importing individual operators vs all of Rx

jayphelps commented 8 years ago

Yep, indeed it still does

image

Meligy commented 8 years ago

The other interesting example is default import vs normal import.

I expect this line to load the entire library:

import Rx from "rxjs/Rx";

but I get the impression this line doesn't act like it loads the entire library (although it actually does):

import {Observable} from "rxjs/Rx";

because if I try map for example, I still need to import like this:

import {Observable} from "rxjs/Rx";
import 'rxjs/add/operator/map';

Or like this:

import {Observable} from "rxjs/Rx";
import "rxjs/Rx"; // same as: import Rx from "rxjs/Rx";

Tree Shaking

All the previous examples were in TypeScript with RXJS beta 6 (Angular 2 apps), but in a simple example using babel and Webpack 2 beta 21, this app:

import { Observable } from 'rxjs/Rx';

Observable
    .from([1,2,3])
    .map(function(n) { return  "Number " + n; })
    .subscribe(console.log);

This loads the entire library as I can tell from file size, but running the bundle file throws an exception because it doesn't see the from static or map operator.

To fix it, as above, I need to either import "rxjs/Rx"; / import Rx from "rxjs/Rx";, or:

import 'rxjs/add/observable/from';
import 'rxjs/add/operator/map';

Now, I get a working app that loads the entire library in its size still:

import { Observable } from 'rxjs/Rx';

import 'rxjs/add/observable/from';
import 'rxjs/add/operator/map';

Observable
    .from([1,2,3])
    .map(function(n) { return  "Number " + n; })
    .subscribe(console.log);

BUT, if I change the first line to import { Observable } from 'rxjs/Observable';, the program still works after bundling, but the size drops significantly, showing that tree shaking is working.

jayphelps commented 8 years ago

@Meligy Best I can tell, all the examples you showed appear to be bugs, either with RxJS or the bundlers.

import { Observable } from 'rxjs/Rx';

Observable
    .from([1,2,3])
    .map(function(n) { return  "Number " + n; })
    .subscribe(console.log);

should work in both JavaScript + webpack and TypeScript.

Looking inside the rxjs npm bundle for beta.6 and beta.11, everything seems in order as it should. Hard to say without a local example to examine.

Meligy commented 8 years ago

Here's my demo repo for the babel / beta 12 version:

https://github.com/Meligy/rxjs-treeshaking

Meligy commented 8 years ago

For Babel, the normal scenario seems to work well:

import { Observable } from 'rxjs/Rx';

Observable
    .from([1,2,3])
    .map(function(n) { return  "Number " + n; })
    .subscribe(console.log);

So, this bit at least is good and makes sense.

I think I got it confused because in TypeScript it doesn't work. Probably a typings issue. I have had it only with beta 6 though in Angular apps so I'll test this again with beta 12.

One thing to mention about the above example though is that it's of course not tree-shakable.

anaisbetts commented 8 years ago

Is tree-shaking what JavaScript people call linking?

jayphelps commented 8 years ago

@paulcbetts typically in JS world it refers to removing code that isn't used, but otherwise would be included because of the limitations of statically analyzing historical JavaScript module systems. Although I had heard the term long before Rollup and outside of JavaScript, arguably I think it popularized it.

Outside of JS it generically means exactly what the analogy sounds like. "Shaking" some tree of nodes to remove ones that were about to fall off anyway.

jayphelps commented 8 years ago

@Meligy thanks for the example repo. I'll check it out.

As an aside, there really isn't much to safely tree-shake of RxJS because operators are defined on the Observable prototype. I haven't followed webpack2 or rollup closely enough to know if they're attempting to detect that kind of usage, which would be extremely tough to do safely in a non-typed language IMO (but hey, there are people much smarter than me, so anythings possible). That is why RxJS supports adding the operators individually, so you can reduce bundle size that way. Often, people create a single file where they import all the operators they need for their entire project and then import that file in their main entry point. That way you don't have to defensively import the same operators every time they use them throughout.

People often obsess over bundle size prematurely as well. Every project has some unique usage to it, but many of them can import all of RxJS and have a perfectly acceptable bundle size if they're gzipping. (or obviously if they're an electron app, it's #YOLO)

benlesh commented 7 years ago

This has been inactive for quite some time. Is this still something we want to do? I'll close for now, and we can reopen if so.

lock[bot] commented 6 years ago

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.