Using more than one form of imports causes multiple copies/identities of Observable

[jayphelps]

alternative title for SEO: "all operators are undefined, not on the prototype"

RxJS version:
5.5.0
Code to reproduce:
Use two different styles of imports in the same project.

import 'rxjs';
import { Observable } from 'rxjs/Observable';

console.log(Observable.prototype.map);
// undefined

Or

import * as Rx from 'rxjs';
import { Observable } from 'rxjs/Observable';

console.log(Rx.Observable === Observable);
// false

Expected behavior:

They use the same copy of rxjs, so that there is only one Observable identity. Adding operators individually via import 'rxjs/add/operator/map' or import 'rxjs' both patch the same Observable.prototype as import { Observable } from 'rxjs/Observable'

Actual behavior:

They read from different copies of rxjs in the same package so adding operators to one doesn't add them to the other.

This becomes a major issue when dealing with libraries like angular, redux-observable, etc which use import { Observable } from 'rxjs/Observable' but the consuming devs use import 'rxjs'. None of the operators get added because they are not the same Observable.

This was first reported in redux-observable/redux-observable#341 by several users, but I'm also experiencing it as well.

Additional information:

You won't reproduce this using CJS or with Babel. You need a bundler that supports tree shaking via "module". See #2984 (comment) for my working theory on the cause.

Workaround

The workaround is to use "rxjs/Rx" instead of "rxjs".

import 'rxjs/Rx';
import { Observable } from 'rxjs/Observable';

console.log(Observable.prototype.map);
// [object Function]

This works because then you won't using the ES modules copy, instead using the default CJS code--this also means you won't get what limited tree shaking you might have gotten, but you didn't get that before 5.5.0 and because operators are on the prototype they are not shaken, so the difference is modest. So using this workaround will behavior identical to how it was in 5.4

Cc/ @jasonaden @IgorMinar @benlesh

[jasonaden]

@jayphelps What package.json file are you referring to? The one that's distributed with rxjs points to './Rx.js. And the index.js file you're referring to is there in the source code version of Rx. This is what you would get if you did npm install @reactivex/rxjs.

I believe you are depending on rxjs though, right? Not building from source?

Can you send a simple repro case? I'll take a look, and hopefully we can get it fixed quick enough for 5.5.1.

[jayphelps]

What package.json file are you referring to? The one that's distributed with rxjs points to './Rx.js. And the index.js file you're referring to is there in the source code version of Rx. This is what you would get if you did npm install @reactivex/rxjs

Sorry for jumping the gun. I was quickly checking "what changed" recently and that was just one thing I noticed that seemed highly suspicious. I haven't had the time to truly confirm whether it's related. If your change only relates to @reactivex/rxjs then it's unrelated indeed, assuming it didn't have any unintended effects with the rxjs package. I've reworded my thoughts to not sound as confident in the cause 👍

I believe you are depending on rxjs though, right? Not building from source?

Yup. Standard rxjs package.

Can you send a simple repro case? I'll take a look, and hopefully we can get it fixed quick enough for 5.5.1.

I'll put something together later tonight. I was able to reproduce it in two my apps (and three people in redux-observable/redux-observable#341) but admittedly I haven't had time to try it in a complete empty project with nothing but rxjs. It's also possible this requires using ES2015 module syntax to reproduce, but I doubt it.

[jasonaden]

@jayphelps Okay, sounds good. Send it over when you have that ready and I'll take a look. If a problem surfaced here, I definitely want to get it fixed ASAP. There were some renamed operators, but we should be exporting anything renamed as the old name.

[jayphelps]

Reproduction: https://github.com/jayphelps/rxjs-issue-2984

I figured out the issue though: bundlers that understand the "module" flag in package.json will use a different copy of rxjs than if you use import * as Rx vs operator patching/direct import. i.e. there's a node_modules/rxjs/Observable.js and also a node_modules/rxjs/_esm5/Observable.js.

"module": "./_esm5/Rx.js"

988e1af#diff-e7586ccec757a94ea1ab5c539aa69c3dR35

This is great for tree shaking and obviously done intentionally, but it means you have to use one or the other (or you're stuck with whatever your dependencies like redux-observable choose). Presumably the "es2015" flag + code has the same problem.

It's unclear what the right "fix" or more accurately, choice, should be made. This is definitely a breaking change obviously, so we can revert the behavior for 5.5.1. But for v6 it's unclear if there's a way to make both work. I don't think so.

I've also updated my OP with the workaround of importing from "rxjs/Rx" instead which won't use the esm code.

[cartant]

IIRC, this would not have been a problem in beta.0 as package.json files were used at every level for redirection to _esm2015, etc. However, that effected a case-related problem as the previous version's observable directory and Observable.d.ts file both needed to be represented as directories.

If these conflicts could be addressed, the mechanism used in beta.0 should be fine for v6.

[jasonaden]

@jayphelps Yes, that makes sense now.

So I’m terms of the breaking change, we could remove the module reference from package.json. But I guess a question would be why both are being used? Deep imports and importing the “kitchen sink” version should generally be mutually exclusive.

[jasonaden]

Also, considering these two types of imports generally wouldn’t be used together, how would you feel about the recommendation being to just adjust Webpack module loading sequence to go straight to “main” for rxjs?

The problem with removing “module” is it makes it much harder for people using the kitchen sink import to get the benefits of ESM (significant size reduction).

[jayphelps]

Using both import patterns has been the status quo since v5. It’s been how libraries like angular, redux-observable, ngrx, etc can import Observable directly without also bringing in all of rxjs. Then consumers can choose between patching individual operators or patching the entire kitchen sink. So unfortunately for this issue, they have not been mutually exclusive; in fact, having both was the intent from the get go.

Also, considering these two types of imports generally wouldn’t be used together, how would you feel about the recommendation being to just adjust Webpack module loading sequence to go straight to “main” for rxjs

I’m not sure what you mean, can you clarify?

[jayphelps]

While we discuss future options, is there any objections by anyone to removing ”module” and releasing 5.5.1?

[cartant]

I think that some consideration needs to be given to the fact not only is this a breaking change, but it's also one that's hard to diagnose. It's only going to effect strange, runtime behaviour.

Just my two cents, but I'd vote for removing it.

[jasonaden]

So my argument for not using both is that by importing rxjs/Rx you’re going to get the entire library. Nothing will be treeshakable no matter what you do.

That import should only be used in situations like sample apps where you don’t care about bundle size.

[jayphelps]

The issue applies to operator patching in general, even individually. This is my understanding: since traditionally rxjs has used prototype-based operators which are not treeshakeable by current bundlers (ignoring Closure Compiler with annotations and unsafe removals) normal tree shaking hasn’t been a priority because the only things it can shake are classes (and likely some random things like some of the schedulers. Removing those kb is meaningful, but most people asking for shaking wanted it to be done on the operators. This is one reason lettable operators were conceived, because then they’re shakeable.

Angular and other libs import Observable directly and then those libraries let you in your app patch all operators with import ‘rxjs' or they might advocate devs patch operators individually. The “patch all” way just calls imports all the individuals for you, but not letting you control size, obviously.

That said, I haven’t been on the core team for a while so I’ll defer to them to elaborate further and add any new details I haven’t heard of. 🙃

[jasonaden]

Generally that’s correct. The problem is that by importing rxjs/Rx or rxjs directly, you get every static method for observable, every operator, every scheduler, etc. Essentially you get the entire weight of the library.

Therefore there’s never a scenario where importing from rxjs/Rx as well as going deep imports to pull in operators gives any value.

I’ll ping Ben and OJ on this to weigh in. I’m fine either way. It just means people importing through rxjs/Rx won’t get the advantage of a smaller bundle.

Also, to be clear, even when using the “module” import for the ESM version, nothing is treeshakable. However, there’s a large size difference between CJS modules (the “main” key) and ESM to the tune of 10’s of kb with Webpack 2 and over a hundred with Webpack 3’s scope hoisting. So those are “free” bytes by keeping the “module” key there.

[cartant]

This issue is likely the cause of the problems in #2977, as the project in the related Angular issue includes the following imports (in addition to deep imports):

import { Observable, Subscription } from 'rxjs';
import { Observable } from 'rxjs';
import { Subscription } from 'rxjs';
import { Subscription, BehaviorSubject } from 'rxjs';
import { Observable, Subscription } from 'rxjs';

[jasonaden]

@cartant Good point on the related Angular issue. I was looking at it from the context of a single project, not the combination of different projects importing different ways.

I’ll push a commit to remove the “module” key.

[benlesh]

@jasonaden unfortunately this is a breaking change. We'll need to fix it. The nature of the patching operators has always meant that the expectation is if you import from rxjs/Observable you should get the same reference as you would get from rxjs or rxjs/Rx. They are definitely not mutually exclusive, as I have seen an unfortunate amount of people using both.

We are definitely going to need to fix this.

[jayphelps]

Good point on the related Angular issue. I was looking at it from the context of a single project, not the combination of different projects importing different ways.

lol this is what I was trying to say. Sorry I wasn’t clear! 😆

[montella1507]

Guys, thank you very much for reviewing my issue in Angular project. I hope it helped to find the issue.

[jasonaden]

@jayphelps Yeah, sorry... I realized eventually ehats what you were saying. I missed it reading all this on my phone. I’ll send the PR in a few minutes.

[unlight]

// RxJS since 5.5+
import { Observable as Observable1 } from 'rxjs';
import { Observable as Observable2 } from 'rxjs/Observable';
console.log(Observable2 === Observable1); // => false, but expected true

Fix on webpack side:

resolve: {
    alias: {
        rxjs$: 'rxjs/_esm5/Rx.js',
        rxjs: 'rxjs/_esm5'
    }
},

Full config and repo example

[IgorMinar]

Wasn't this fixed in 5.5.1?

…

On Thu, Oct 26, 2017, 4:01 PM Roman Vasilev ***@***.***> wrote: // RxJS since 5.5+ import { Observable as Observable1 } from 'rxjs'; import { Observable as Observable2 } from 'rxjs/Observable'; console.log(Observable2 === Observable1); // => false, but expected true Fix on webpack side: resolve: { alias: { rxjs$: 'rxjs/_esm5/Rx.js', rxjs: 'rxjs/_esm5' } }, Full config <https://github.com/unlight/webpack-resolve-alias-playground/blob/master/webpack.config.ts> and repo example <https://github.com/unlight/webpack-resolve-alias-playground> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6CHjtoBdmuHJ4gjLvI4pgMtuSVHJks5swJDPgaJpZM4QBQvp> .

[lock]

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.