Skip to content

Add prototype-patching operator modules #750

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
jeffbcross opened this issue Nov 18, 2015 · 7 comments
Closed

Add prototype-patching operator modules #750

jeffbcross opened this issue Nov 18, 2015 · 7 comments
Milestone

Comments

@jeffbcross
Copy link
Contributor

(Recapping some offline discussion between @Blesh, @IgorMinar, @robwormald and myself)

In order to allow exporting a minimal Observable and to increase composition of operators, while maintaining nice chaining syntax, we propose exporting each operator as an easily-importable module that will have a side effect of patching the Observable prototype.

import {Observable} from '@reactivex/rxjs';
// Observable.prototype.map = undefined
import {map} from '@reactivex/rxjs/map';
// Observable.prototype.map = function (...) {...}

With TypeScript 1.8, there will be support for this type of module to re-open and extend types, so we can not have to stub out all types on the Observable class like we do now. In anticipation of this change, we should go ahead and add interfaces for all operators, with optional methods. The Observable class will implement all operator interfaces instead of manually stubbing out methods on the class itself.

It's not ideal for an import to have side effects, and to mutate a shared object. In this case, I think the mutation is not likely to cause issues for people, since most folks will only import operators in one place inside an application. And if folks are only importing operators from rxjs, there's no risk of method name collision.

Eventually, when JS has sugar like function bind to make extension-like functions more composable, there should be a breaking change to remove the side effect of the imports.

@jeffbcross
Copy link
Contributor Author

CC @Blesh @robwormald

@benlesh
Copy link
Member

benlesh commented Nov 18, 2015

It's obvious how this will benefit browser end-users, but what about the node users? While updating window in browsers is (mostly) acceptable, updating global in node is not. However, I would expect node users to mostly import the whole kitten-kaboodle and not care about size.

Also: What are alternatives we've considered?

@robwormald
Copy link
Contributor

I think from the outside, the story doesn't really change for a node user:

before:

//Rx.ts
import {Observable} from './Observable'
import {map} from './operators/map'
//etc

Observable.prototype.map = map;

export {Observable}

after:

//Rx.ts
import './operators/map'
import {Observable} from './Observable'
//etc

export {Observable}

browser user who wants the whole mess:

import {Observable} from 'rxjs/Rx';

browser user who only wants map

import {Observable} from 'rxjs/Observable'
import 'rxjs/operators/map'

node user who wants the whole mess

var Rx = require('rxjs') //package.json maps to main: Rx.js

I just tested that and it appears to work fine. Barring that, index.js could do what we're doing today, as browers don't typically understand the index concept.

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

@robwormald what is the status on this?

@robwormald
Copy link
Contributor

About 20 minutes away

On Nov 30, 2015, at 12:07 PM, Ben Lesh [email protected] wrote:

@robwormald https://github.com/robwormald what is the status on this?


Reply to this email directly or view it on GitHub #750 (comment).

@benlesh
Copy link
Member

benlesh commented Nov 30, 2015

👍

@jeffbcross
Copy link
Contributor Author

Closed by @robwormald in d17a42f

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

3 participants