The big reorganization PR for v6 (WIP)

[benlesh]

See commit messages for more details.

• Exports everything from top level rxjs.
• Pipeable (lettable) operators import { map, filter, scan, switchMap } from 'rxjs';
• Observable creation helpers import { timer, concat, fromEvent } from 'rxjs';
• Removes operator versions of observable creation helpers (use concat(a, b, c) instead of a.pipe(concat(b, c)))
• Starts moving things people should not be importing directly into an "internal" directory, so it's more obvious you're doing something silly.

[rxjs-bot]

	Warnings
⚠️	❗ Big PR (1)

	Messages
📖

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

CJS: 1388.1KB, global: 749.5KB (gzipped: 120.5KB), min: 145.4KB (gzipped: 31.4KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage decreased (-0.03%) to 97.049% when pulling 60de136 on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.03%) to 97.051% when pulling 8879a32 on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.03%) to 97.053% when pulling f8aae21 on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.03%) to 97.053% when pulling f8aae21 on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.3%) to 96.809% when pulling 9d6b8bb on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.3%) to 96.742% when pulling 4a58200 on benlesh:reorg into 3390926 on ReactiveX:master.

[kwonoj]

Assigning myself to give a quick peek.

[coveralls]

Coverage decreased (-0.03%) to 97.053% when pulling 01c7411 on benlesh:reorg into 3390926 on ReactiveX:master.

[coveralls]

Coverage decreased (-0.03%) to 97.067% when pulling 8f5e263 on benlesh:reorg into 57d7b97 on ReactiveX:master.

[coveralls]

Changes Unknown when pulling 8bb23d1 on benlesh:reorg into ** on ReactiveX:master**.

[coveralls]

Coverage decreased (-0.03%) to 97.067% when pulling 8bb23d1 on benlesh:reorg into fdfeef2 on ReactiveX:master.

[rgbkrk]

I thought the transition from v4 -> v5 was good, this is even better. So much easier to navigate what's being exported, the interfaces make so much sense, and I'm looking forward to introducing more people to Rx with how much simpler the imports read. I'm loving this switchover from v5 -> v6.

[kwonoj]

Initial high level it looks fine, I guess there may be some glitches but we can fix it since this is for non stable builds.

[benlesh]

Would it be helpful to have the creation methods named something like: rxMerge, rxConcat, rxFrom, rxFromEvent, etc? or fromConcat, fromMerge, fromForkJoin fromRace, etc? or obserfvableMerge, observableConcat, observableOf?

I feel like the latter is most descriptive, but too verbose. And that's coming from a guy that can type the word "observable" blindfolded. (irony: I typo'ed "observable" three times just there)

And then still have a single import location?

[felixfbecker]

I don't really see a problem with having the source of the operators be in many files, but then have an index.ts that does export * from for all of them. That's a very common pattern. Most people will import from the root, but some may not have a bundling process with tree shaking. I feel like the index.ts shouldn't contain all that code, but also deep imports shouldn't be a requirement for all users.

Would it be helpful to have the creation methods named something like: rxMerge, rxConcat, rxFrom, rxFromEvent, etc? or fromConcat, fromMerge, fromForkJoin fromRace, etc? or obserfvableMerge, observableConcat, observableOf?

This is definitely a question I have asked myself. Just using of or concat in your code sounds very generic. At the same time, that's true for all modules and it shouldn't be the module's responsibility to prefix with the module name like rx - folks can always alias or do import * as rx.
In a codebase that heavily uses rxjs it definitely reads nicer to just combineLatest instead of observableCombineLatestFrom. If there is a naming conflict, folks can use import aliases for those instances.

[wardbell]

My instincts lie with @IgorMinar. Renaming creation operators will make migration harder and still leave you with something to explain.

How many kinds are there? Creators/Observables, Operators, Subscribers, and Schedulers as I recall. And then there are Utilities right? That’s five.

So is n===5? I guess that’s livable.

One big rxjs bucket is my least favorite because there are just too many kinds of things in there that are easy to confuse.

Adding prefixes (such as “rx” or “from”)will make migration harder and is just another way of saying “These things belong in the same bucket.” If so, give them a bucket.

Bikeshedding perhaps. It’s a great direction however you decide.

[IgorMinar]

I should also point out that the small N paths solution doesn't suffer from ergonomics issues of introducing a new import line, because most editors and IDEs do that for you automatically.

If you however use an editor that can't create imports for you via autocompletion then using a single path is obviously more ergonomic.

The question is if optimizing the api surface for code authoring using an editor that has no ES import support is the right goal. I don't think it is, but that's my personal opinion 😄

I believe that this is an example of authoring workflow most rxjs developers deserve:

source: https://marketplace.visualstudio.com/items?itemName=steoates.autoimport (note webstorm and other editors, support this workflow as well)

[IgorMinar]

I forgot to mention that you should notice how the autocompletion in the screencast above highlights the path the symbol is coming from. This is what helps me pick the right symbol and in case of rxjs would help me distinguish between creation methods and operators. Once I make the choice, I no longer see the path (except) in the import, which removes clutter (unlike the "rx" prefix which would remain in the code even after I pick the symbol)

[felixfbecker]

@IgorMinar I get that argument and I personally use an editor that has that feature, but I would still prefer to not have the top of every file look like this (as atm):

So not adding 1 import per operator is a valid goal no matter of having an IDE or not

[felixfbecker]

Btw, static operators like concat could still be used as an operator by currying it:

of(1, 2, 3).pipe(
  $ => concat($, of(4, 5, 6))
)

[IgorMinar]

@felixfbecker most of those import lines are because each operator is imported separately. my proposal would reduce that snippet to two imports (from 'rxjs' for Observable and creation menthods, and from 'rjxs/opeartors' for the operators).

Schedulers and other symbols are used very rarely so 99% of the code should not need to import them.

[acutmore]

Controversial:
Single import path & operators all start with a common prefix

I think the discoverability of operators via IDE auto complete is critical. Gives back what we lose moving away from proto and towards pipes.

import { of, concat, $map, $filter } from ‘rxjs’;

of(1).pipe(
  $map(v => v + 1),
  $ => concat($, of(3),
  $filter(v => v > 2),
)

[SanderElias]

I'm on the same path as @IgorMinar.

Personally, I would love to rename the creation methods as this would make it way more explicit what they are doing. However, as that is breaking backwards compatibility too much I don't think its a good idea. Adding aliases would sadly only add to the confusion.

When that's from the table, from 'rxjs' for everything except for the operators that will come from 'rxjs/operators' seems the most straightforward.
As it separates what goes on the "inside" and "outside" of the pipe, it will relieve some of the confusion to new(ish) devs coming to RxJs.

[myspivey]

I back @IgorMinar mostly as well although I would argue that as this is a major version, I think pulling the trigger and renaming is cleaner. I like the from* options as that is very descriptive to what you are doing and reduces confusion between the operators. Sure we can import from two places but that then leaves code readability up to how it was imported and can create collision and confusion. If I am adopting a new major version I know things are going to break. When I switched to lettables I had to go through and rename catch and do in many places, why would this be any different. That is just my own feelings personally. Overall amazing work and looking forward to seeing this in the wild.

[jichang]

I think single import path will be fine for TypeScript developers, as the type will tell the difference between operators and create function, but for JS developers, especially for beginners, single import will create some confusion, so agree with @IgorMinar

[wmaurer]

I'm for importing from either a single entry point (1st preference) or ~5 paths (2nd preference).

I would also welcome renaming creation operators to rxMerge, rxConcat, etc. I like the naming distinction from lettable operators.

I think renaming the creation operators has a relative minor impact on existing code bases (search/replace). The bigger impact is removing the lettable operator equivalents. I now use the creation methods exclusively now, but our older code does not. Changing from operators to create methods involves more shifting of code around.

[benlesh]

It seems like the vocal people here would like to divide things up in terms of functionality to some degree. This would give us:

• rxjs - for types like Subject, Observable, BehaviorSubject, etc
• rxjs/create - for of, from, fromEvent, concat, combineLatest, etc
• rxjs/operators - for map, switchMap, etc
• rxjs/util for utilities like noop and pipe (After typing "noop" I typed "poop" instead of "pipe" the first time)
• rxjs/scheduler for the schedulers
• rxjs/symbol for the symbols such as Symbol.observable

Is the compromise proposal to have:

• rxjs/operators - map, switchMap, etc
• rxjs - everything else

??

I'm going to finish up this PR today and merge. This has taken too long.

I have to be honest, I really don't like it. I'd just assume have one entry point.

(EDIT: Got a few DMs that I sounded "angry"... I'm not at all.. just bad text maybe? haha)

[kylecordes]

@benlesh Your "one" notion has much wisdom, is very compatible with the future of bundling tools, is easy to consume, etc. The only downsides, discussed here, are that the list of things exported from that one place can get a little long (hard to look through it by hand), and names have to be deconflicted. Trust your heart :-)

[benlesh]

Well the fact is, even if we split them up, there's going to be a problem with concat.

import { concat } from 'rxjs';
import { concat } from 'rxjs/operators';

... you'll need to alias regardless. What to, though? fromConcat? If that's the case, should we just rename it? Are there compelling cases why: a.pipe(concat(b, c)) is better than concat(a, b, c)? Perhaps a.pipe(switchMap(fn), concat(b, c)) is better than concat(a.pipe(switchMap(fn)), b, c)? Doesn't read nicely to me.

[jayphelps]

@benlesh

I have to be honest, I really don't like it. I'd just assume have one entry point.

For me at least, the idea was primarily to have rxjs/operators and rxjs/observables (or whatever). The other categories we could do, or we could just put others in the root as they're so rarely imported that I doubt it makes much of a difference.

Well the fact is, even if we split them up, there's going to be a problem with concat.

I thought the operator variants were going away?

Removes operator versions of observable creation helpers (use concat(a, b, c) instead of a.pipe(concat(b, c)))

[benlesh]

@jayphelps

I thought the operator variants were going away?

That's what I'd like, but I felt like there is a subtext here to keep them. I'm probably mistaken. If so, that's great.

[kylecordes]

@benlesh It always seemed odd to have two ways to do concat and friend. Yet some people rely on them. I think it is justifiable on a major new version, to tighten up the core API, and move "pipeable concat" over to a secondary support package, to help out the folks would would miss it if it were gone.

[benlesh]

So:

• rxjs/operators - map, switchMap, etc
• rxjs/observable - from, of, fromEvent and friends
• rxjs - junk drawer? Just classes? Should Schedulers go in here? What about utility functions like identity, noop and pipe?

[rgbkrk]

I think it's cool to put identity, noop, and poop pipe at the top level.

[kylecordes]

Speaking in favor of what Ben said up a ways (just one package): It seems silly to have a rxjs/observable. Observables are what RxJS is all about.

[rgbkrk]

Speaking in favor of what Ben said up a ways (just one package): It seems silly to have a rxjs/observable. Observables are what RxJS is all about.

Yeah, I liked the rxjs/create name.

[amoerie]

If I can chime in here: I would vote for importing everything from "RxJs". I don't agree that this would be very confusing, because there are types and documentation for everything. (Thanks btw!) If anything, I'm afraid that having multiple import paths deviates from the norm and would be worse complexity wise! When in doubt, go for simple..

Just my 2 cents..

[benlesh]

Moving this pr to #3223 ... it's the same PR I just had a problem with my fork. (Sorry).

[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.