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

fix(distinctKey): Removed accidental leftover reference of distinctKey #2162

Merged
merged 1 commit into from
Nov 30, 2016

Conversation

jayphelps
Copy link
Member

Primarily the issue is src/add/operator/distinctKey.ts which imported the non-existing operator, but other documentation references still existed too.

Fixes #2161

Cc/ @alexeagle

`distinctKey` was removed in the previous version but we accidentally
left some files and references to it, which can cause build failures for
TS users.

fixes ReactiveX#2161
jayphelps referenced this pull request Nov 30, 2016
… perf improvements (#2049)

* feat(distinct): remove `distinctKey`,  `distinct` signature change and perf improvements

- Adds a limited Set ponyfill for runtimes that do not support `Set`
- `distinct` now supports an optional keySelector argument that the user can use to select the value to check distinct on
- `distinctKey` is removed as it is redundant
- `distinct` no longer supports a comparer function argument, as there is little to no use case for such an argument that could not be covered by the keySelector
- updates tests to remove tests that do not make sense and test new functionality

BREAKING CHANGE: `distinctKey` has been removed. Use `distinct`
BREAKING CHANGE: `distinct` operator has changed, first argument is an
optional `keySelector`. The custom `compare` function is no longer
supported.

resolves #2009

* perf(distinct): increase `distinct()` perf by improving deopts

- moves keySelector call to a different function with a try catch to improve V8 optimization
- distinct calls with no keySelector passed now take a fully optimized path, doubling speed again

related #2009

* docs(distinct): update distinct docs to fit new API
Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 LGTM

@coveralls
Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage remained the same at 97.686% when pulling 9fd8096 on jayphelps:removed into f51b8f9 on ReactiveX:master.

@kwonoj
Copy link
Member

kwonoj commented Nov 30, 2016

approval checker not passed? 🤔 LGTM (again)

@jayphelps
Copy link
Member Author

@kwonoj can I get a merge too plz?

@kwonoj kwonoj merged commit bd56b3c into ReactiveX:master Nov 30, 2016
@kwonoj
Copy link
Member

kwonoj commented Nov 30, 2016

@jayphelps ABSOULTELY my pleasure.

@lock
Copy link

lock bot commented Jun 6, 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 6, 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

Successfully merging this pull request may close these issues.

3 participants