Skip to content

Add prototype-patching operator modules #843

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

Closed
wants to merge 3 commits into from

Conversation

robwormald
Copy link
Contributor

Step one (broken into two commits - operators and statics) in modularization strategy.

Moves prototype patching into individual modules, so Observable prototype can be patched incrementally.

Before:

import {Observable} from '@reactivex/rxjs'; //ALL THE THINGS
//or
import {Observable} from '@reactivex/rxjs/Observable'; //NONE OF THE THINGS

After:

import {Observable} from '@reactivex/rxjs/Observable'; //NONE OF THE THINGS
//SOME OF THE THINGS!
import '@reactivex/rxjs/observables/PromiseObservable';
import '@reactivex/rxjs/operators/map';
import '@reactivex/rxjs/operators/filter';
import '@reactivex/rxjs/operators/reduce';

Part two will be using TS 1.8's module-reopening functionality to patch the interface, at which point the stub methods will be removed from Observable.ts. Not implemented yet - see microsoft/TypeScript#5269 (comment)

Current side effect (minor) is that type-completion will show methods that have possibly not been imported yet.

Closes #750

@robwormald
Copy link
Contributor Author

cc @IgorMinar @jeffbcross - i couldn't work out optional syntax here, might be the pumpkin pie hangover.

Change from monolithic entry points to individual operators patching the protype.
@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

//SOME OF THE THINGS!
import '@reactivex/rxjs/observables/PromiseObservable';

This confuses me. Is this to say that fromPromise doesn't exist until PromiseObservable is imported? While I'm generally in favor of this approach for operators. I think for the static methods it's harder to figure out.

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

Perhaps it should be:

import `@reactivex/rxjs/observable/fromPromise`;

(NOTE: The reactivex/rxjs will shrink to just rxjs at some point)

@robwormald
Copy link
Contributor Author

Yes, correct, it doesn't exist until imported.

Definitely like this better, it reads much more nicely 👍

import `@reactivex/rxjs/observable/fromPromise`;

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

I'd go so far as to have it also export the fromPromise function (same for the others).

So:

import {fromPromise} from 'rxjs/observable/fromPromise';

fromPromise(makePromise()).subscribe();

or

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

Observable.fromPromise(makePromise()).subscribe();

would both work.

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

... well I guess it could export PromiseObservable... I dunno. As long as the name is easier to read.

@trxcllnt
Copy link
Member

@Blesh:
I'd go so far as to have it also export the fromPromise function (same for the others). [snip] ... well I guess it could export PromiseObservable... I dunno. As long as the name is easier to read.

Could we export both fromPromise and PromiseObservable from that module, or would that go against our "only export one thing per module" rule?

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

Could we export both fromPromise and PromiseObservable from that module, or would that go against our "only export one thing per module" rule?

No such rule, IMO. So I don't see why not.

@robwormald
Copy link
Contributor Author

observables => observable ✔️

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

LGTM.

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

Merged as of 95279cc... VERY nice work, @robwormald, and great idea overall @IgorMinar @jeffbcross et al.

@benlesh benlesh closed this Nov 30, 2015
@kwonoj
Copy link
Member

kwonoj commented Nov 30, 2015

👍 yay!

@robwormald robwormald deleted the refactor/patch-proto branch November 30, 2015 23:30
@robwormald
Copy link
Contributor Author

@trxcllnt i like the idea of exporting fromPromise et al, but they're mostly static methods on classes (PromiseObservable.create) - should i just

const fromPromise = PromiseObservable.create;
export {fromPromise}

or is there something more clever i'm missing?

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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

Successfully merging this pull request may close these issues.

4 participants