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

Provide side-effect-free version of operators #860

Closed
jeffbcross opened this issue Dec 2, 2015 · 19 comments
Closed

Provide side-effect-free version of operators #860

jeffbcross opened this issue Dec 2, 2015 · 19 comments
Assignees
Labels
feature PRs and issues for features

Comments

@jeffbcross
Copy link
Contributor

Right now each of the operator modules contain both the operator implementation AND the Observable-patching side effect logic. This prevents smart people like me from being able to safely import an operator without polluting the Observable prototype.

I think the right default behavior is to keep the side-effect-producing modules named as they already are, in the operators folder, and import from a "raw" module that exports the operator implementation.

I propose adding an implementation folder inside operators to contain actual implementations.

|- rxjs/
|-- operators/
|---- implementation/
|------ map.ts - actual implementation with no side effects
|---- map.ts // Just imports the raw `map` operator and patches it onto Observable

CC @robwormald @Blesh @IgorMinar

@jeffbcross jeffbcross added the feature PRs and issues for features label Dec 2, 2015
@jeffbcross jeffbcross self-assigned this Dec 2, 2015
@benlesh
Copy link
Member

benlesh commented Dec 2, 2015

I thought about this last night. In Babel, I'd much rather import the operator and use myObservable::operator() directly.

I'm generally for this change, but I think that implementation is a lot to type. impl is more palatable for me.

What if it were something like with or add or patch? Something that described what the import was doing:

import {Observable} from 'rxjs/Observable';
import 'rxjs/with/map';
import 'rxjs/with/filter';

and for direct operator imports:

import {Observable} from 'rxjs/Observable';
import {map} from 'rxjs/operator/map';
import {filter} from 'rxjs/operator/filter';

@jeffbcross
Copy link
Contributor Author

@Blesh I'm in favor of flipping so that the side-effect free operator is in rxjs/operators/ and the patching-version is in rxjs/operators/${subfolder}. This is more forward-compatible with a future when we no longer need to rely on monkey patching. The subfolder needs a less verby name though.

@benlesh
Copy link
Member

benlesh commented Dec 2, 2015

The subfolder needs a less verby name though.

But the act of importing anything under it is literally acting like a function with side-effects. It's executing code that has side-effects.

rxjs.addMap() vs rxjs/add/map so to speak.

@jeffbcross
Copy link
Contributor Author

@Blesh I more meant that literally being a verb felt wrong to me, but a noun version of the verb would feel better. But I don't really have a standard or convention to point to to justify my feelings, and I haven't thought of a better name (other than monkey-patchers)

@IgorMinar
Copy link
Contributor

I'm fine optimizing for the future and moving the modules with side-effects into a separate directory.

so rather than impl we would have patch or similar directory that we would use until function bind is a thing. how about that?

@IgorMinar
Copy link
Contributor

add as suggested by @Blesh is even better than patch

@jeffbcross
Copy link
Contributor Author

@IgorMinar no no no, go back to what you suggested with patch. Patch is beautiful because it's a verb AND a noun.

@jeffbcross
Copy link
Contributor Author

I realize @Blesh proposed patch in his original comment, and I've now come around to liking patch.

@IgorMinar
Copy link
Contributor

rxjs/add/map reads as add map to rxjs.

rxjs/patch/map reads as patch map to rxjs.

I guess both are fine. "add" is one char shorter.

I'm fine with either. Let's have Ben decide.
On Tue, Dec 1, 2015 at 5:32 PM Jeff Cross notifications@github.com wrote:

I realize @Blesh https://github.com/blesh proposed patch in his
original comment, and I've now come around to liking patch.


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

@benlesh
Copy link
Member

benlesh commented Dec 2, 2015

LOL. Anyone else have an opinion? @kwonoj? @trxcllnt? Haha

@kwonoj
Copy link
Member

kwonoj commented Dec 2, 2015

One of my biggest weakness is always about naming class, variable, module, etcs whatever, especially in English 😓 . Personally bit prefer patch though it's somewhat longer than add. (or mutate? update? not sure.)

@benlesh
Copy link
Member

benlesh commented Dec 2, 2015

Okay, my pick is add:

Reasons:

  1. 2 keys
  2. 3 characters

... also I'm hope to make a BurgerObservable at some point with operators for lettuce, tomato, pickle, etc. Using "patch" will make that less fun.

@trxcllnt
Copy link
Member

trxcllnt commented Dec 2, 2015

LGTM

@jeffbcross
Copy link
Contributor Author

It is Decided.

jeffbcross added a commit to jeffbcross/RxJS that referenced this issue Dec 3, 2015
This change adds a subfolder inside of operator which contains
a module to correspond with each operator, which when imported,
will automatically patch the corresponding operator to the
Observable prototype.

Closes ReactiveX#860
jeffbcross added a commit to jeffbcross/RxJS that referenced this issue Dec 3, 2015
This change adds a subfolder inside of operator which contains
a module to correspond with each operator, which when imported,
will automatically patch the corresponding operator to the
Observable prototype.

Closes ReactiveX#860
@benjamingr
Copy link
Contributor

As an alternative, you can expose patching as separate functionality for observables.

import {op1} from "rx/op1";
import {op2} from "rx/op2";
Observable.add({op1, op2});

Isn't that bad, it means the logic for adding the method to the observable prototype is kept in one place and we're exporting functions that can be used with ::.

I don't feel strongly about it - just want to mention it's a viable alternative here.

jeffbcross added a commit to jeffbcross/RxJS that referenced this issue Dec 4, 2015
This change adds a subfolder inside of operator which contains
a module to correspond with each operator, which when imported,
will automatically patch the corresponding operator to the
Observable prototype.

Closes ReactiveX#860
@benlesh
Copy link
Member

benlesh commented Dec 4, 2015

@benjamingr wouldn't that just be:

Object.assign(Observable.prototype, { op1, op2 })

The sucky thing is no matter how you do this, if you want modular operators, you end up mutating the prototype.

The only other thing I can think of is some sort of static observable factory method to which you pass your operators and it gives you back a new instance of a subclass of Observable... something sort of like:

Observable.factory = (operatorHash, staticMethodsHash) => {
  function DynamicObservable() {
    Observable.call(this);
  }
  DynamicObservable.prototype = Object.create(Observable.prototype);
  DynamicObservable.prototype.constructor = DynamicObservable;
  if (operatorHash) {
    Object.assign(DynamicObservable.prototype, operatorHash);
  }
  if (staticMethodsHash) {
    Object.assign(DynamicObservable, Observable, staticMethodsHash);
  } else {
    Object.assign(DynamicObservable, Observable);
  }
  return DynamicObservable;
}

Then use it like:

const MyObservable = Observable.factory({ op1, op2 });

Still kinda gross, but at least you're not mutating a prototype of a shared class.

@jeffbcross
Copy link
Contributor Author

@benjamingr The primary motivation for the import-based-side-effect approach is that TypeScript 1.8 is planning to support re-opening and adding to types for modules that patch other modules. This is possible because imports must be static, whereas using a method to patch the prototype would not be.

Though I don't think something like add is bad.

jeffbcross added a commit to jeffbcross/RxJS that referenced this issue Dec 4, 2015
This change adds a subfolder inside of operator which contains
a module to correspond with each operator, which when imported,
will automatically patch the corresponding operator to the
Observable prototype.

Closes ReactiveX#860
@benjamingr
Copy link
Contributor

It's just all the code duplication that bothers me. I agree that TypeScript and other type systems are a good enough reason to include it statically though.

@lock
Copy link

lock bot commented Jun 7, 2018

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.

@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
feature PRs and issues for features
Projects
None yet
Development

No branches or pull requests

6 participants