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

Discuss module structure for 6.0 #2912

Closed
benlesh opened this issue Oct 6, 2017 · 8 comments
Closed

Discuss module structure for 6.0 #2912

benlesh opened this issue Oct 6, 2017 · 8 comments

Comments

@benlesh
Copy link
Member

benlesh commented Oct 6, 2017

Issue

Currently users need to import operators and observable creation method methods from a variety of places and it's not always consistent or ergonomic.

import { merge } from 'rxjs/observable/merge';
import { concat } from 'rxjs/observable/concat';
import { timer } from 'rxjs/observable/timer';
import { map, filter, scan } from 'rxjs/operators';

Questions

What is the most ergonomic?

Do we want to try to shoot for an import process like import { map, filter, Observable, Subject, asyncScheduler } from 'rxjs'?

Do we want to keep things mostly the same for now?

How will this affect bundling?

What about node and Electron folks?

Options

  1. Import everything from rxjs.
    • pro: Convenient and ergonomic.
    • con: We'd need to differentiate between static creation functions and operators of the same name (merge and merge might need to be mergeStatic or fromMerge and merge)
       import { merge, fromMerge, map, filter, Observable, Subject } from 'rxjs';
  2. Import all static creation methods and observable types from rxjs, import operators from rxjs/operators.
    • pro:merge and merge can keep their names.
    • pro: We don't have to move the operators.
    • con: two different locations to import from
  3. Import observable types from rxjs observable creation methods from rxjs/observable and operators from rxjs/operators
    • pro: We don't have to move the operators
    • con: three different locations to import from

.. in all cases we should probably still provide the ability to "deep import" operators and the like, as some bundlers still stink at tree-shaking re-export modules.

@benlesh
Copy link
Member Author

benlesh commented Oct 6, 2017

@kwonoj
Copy link
Member

kwonoj commented Oct 6, 2017

For me, importing syntax ergonomics is just good-to-have thing. I have already sufficient numbers of direct path import (import {something} from 'a/b/c') and I don't have strong opinions for those. More interesting point is probably to achieve those what do we need and what we'll lose.

@benlesh
Copy link
Member Author

benlesh commented Oct 31, 2017

Personally, I'm partial to option 1 above, but it would require renaming static creation methods and/or operators... which isn't amazing.

@benlesh
Copy link
Member Author

benlesh commented Oct 31, 2017

cc/ @IgorMinar @jayphelps @mattpodwysocki for thoughts.

@jayphelps
Copy link
Member

jayphelps commented Oct 31, 2017

My preference would be to do away with the lettable forms of operators which have a static equivalent. I imagine this would be quite controversial though... (many of you probably prefer the other form)

import { timer, race, map } from 'rxjs';

const items = race(
  timer(100) |> map(x => x * 100),
  timer(10) |> map(x => x * 10)
);
import { of, concat, map } from 'rxjs';

const items = concat(
  of(1, 2, 3) |> map(x => x * 100),
  of(4, 5, 6) |> map(x => x * 10)
);

I used to use the prototype-based forms (e.g. first.concat(second)) but recently I've settled on always using the static forms as I've found them more far self-explanatory for others reading my code later who may not be as familiar with Rx.

This would reduce the API surface and likely remove a confusion point for some. But it comes at the obvious cost of being less flexible, and perhaps more importantly deviating from some of the other Rx implementations in other languages...though not all of them have the instance variant, and RxJava/RxGroovy call it concatWith. We could call it that, but that doesn't solve the other static operators and I don't personally like it because it has a type signature mismatch with the other -With operator, startWith, which accepts a scalar not an Observable.


Alternatively, there could be two buckets, rxjs/observables and rxjs/operators and when there is a naming conflict the dev has to choose what alias they want to call them.

import { of, concat as concatStatic } from 'rxjs/observables';
import { concat, map } from 'rxjs/operators';

const items = concatStatic(
  of(1, 2, 3) |> map(x => x * 100),
  of(4, 5, 6) |> map(x => x * 10) |> concat(items)
);

In practice I don't think this will happen often--why would someone want to use both forms of them? I'm guessing the answer is "because sometimes it feels better to use one or the other"

@benlesh
Copy link
Member Author

benlesh commented Oct 31, 2017

Honestly, I sorta like @jayphelps' idea here. It's less to maintain, it's cleaner to read IMO also. The only one I don't like moving is concat, but honestly if we had an endWith I'd be fine with it. Then again, I think concat(of(x), source) reads better than source.pipe(startWith(x))

@kylecordes
Copy link

kylecordes commented Nov 19, 2017

I heartily agree with both @benlesh on the notion of "import everything from rxjs", and with @jayphelps on the notion of shrinking the API surface area (as well as the code base itself) by not having to variations of some of these things anymore.

Speaking of static concat vs "startWith", now that the packaging has lessened the need to type the word Observable frequently, there is no longer any significant syntactic penalty for the static form, and the static form expresses the intention more cleanly - the things you are concatenating appear in the source code in the order you are concatenating them. This is easier to teach and learn. it is especially so in this example, but I think the same notion applies to most or all other static versus operator forms of the same underlying operation.

@benlesh
Copy link
Member Author

benlesh commented Oct 2, 2019

This is done.

@benlesh benlesh closed this as completed Oct 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants