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

[Feature Request] Add optional context parameter to operators #1988

Closed
diestrin opened this issue Sep 28, 2016 · 11 comments
Closed

[Feature Request] Add optional context parameter to operators #1988

diestrin opened this issue Sep 28, 2016 · 11 comments

Comments

@diestrin
Copy link

Sometimes the function you want to some operators is not small, or sometimes you just want to reuse that function for multiple operators. Anyway, when you do that you end up by doing something like .map(this.myMethod.bind(this)) or .map(value => this.myMethod(value)).

That's ok, they both work, but if you're using typescript the second will lose the return type, and the second option is too much code repeated (this case is just one param, but what about if you're expecting 2 o 3?)

Instead, it would be really helpful if you add a final optional argument with the context like this .map(this.myMethod, this). This does not introduce any breaking change, and it would be super helpful to keep the typing in typescript.

@diestrin
Copy link
Author

Ok sorry, taking a look at the source code I can see there are some methods that have the thisArg?, but not all of them. Specifically the distinctUntilChange. Is this for any special reason?

@kwonoj
Copy link
Member

kwonoj commented Sep 28, 2016

It's because binding context is being deprecated and supposed to be supported only specific api surface which corresponds to native api such as map. One of major api surface change introducdd in v5 is actually removing thisArg, you can refer detailed discussions at #878.

@kwonoj
Copy link
Member

kwonoj commented Sep 28, 2016

So in case of specific cases of map, it is supported via current impmementation. (https://github.com/ReactiveX/rxjs/blob/master/src/operator/map.ts#L38)

@david-driscoll
Copy link
Member

.map(this.myMethod.bind(this)) will lose the return type. I'm not seeing how .map(value => this.myMethod(value)) would the lose the return type, as it the types should flow very nicely.

As @kwonoj has stated, it's just one less part of the api surface that we're looking to support every where for all operators. With some of the more complicated operators it becomes very hard to model, especially in TypeScript.

Personally I've more or less started just doing fat arrows everywhere, and that seems to work fairly well.

@diestrin
Copy link
Author

@kwonoj a shame you're removing the thisArg :( it doesn't look consistent actually to have some methods with it and some other without it. But I understand your reasons :/

@david-driscoll I didn't say the arrow method was a problem with the types, only bind. The problem with the arrow function (besides the unnecessary closure created) is that you need to copy over all the parameters.

Consider .concatMapTo(obs, (outerValue, innerValue, outerIndex, innerIndex) => this.myCustomConcat(outerValue, innerValue, outerIndex, innerIndex)).
It's just too much code, it doesn't even fit in a single line.

@benlesh
Copy link
Member

benlesh commented Sep 28, 2016

Using bind is more explicit and easier to read for the next developer. The thisArg would look more like a magic argument at first blush.

@benlesh
Copy link
Member

benlesh commented Sep 28, 2016

Regardless, this feature has been discussed in the past, and I don't think we're going to implement it. I do appreciate your point of view and your suggestion, though. If it's okay with you, ins like to close this issue. Please feel free to reopen or comment if you'd like to continue discussion.

@benlesh benlesh closed this as completed Sep 28, 2016
@trxcllnt
Copy link
Member

.concatMapTo(obs, (...args) => this.myCustomConcat(...args))

@diestrin
Copy link
Author

@trxcllnt it won't transmit the types definitions in typescript and will give an error that params does not match

@david-driscoll
Copy link
Member

If you don't use a selector function, the result will be a tuple of all the
input types. You can then destructure the tuple to get the strongly typed
values out.
On Fri, Sep 30, 2016 at 12:16 PM Diego Barahona [email protected]
wrote:

@trxcllnt https://github.com/trxcllnt it won't transmit the types
definitions in typescript and will give an error that params does not match


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1988 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABNdpQdz_5_yYw1WPyUIdZOaFhzPPM9oks5qvTXQgaJpZM4KIUMz
.

Thanks,
David

@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
None yet
Projects
None yet
Development

No branches or pull requests

5 participants