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

feat(smooth): Adds the smooth() operator, the cool operator #2508

Merged
merged 1 commit into from
Apr 1, 2017

Conversation

jayphelps
Copy link
Member

@jayphelps jayphelps commented Mar 31, 2017

Demo: http://jsbin.com/nibuyesofu/2/edit?html,js,output (only works in Desktop browsers. Turn your volume up)
Prior art: https://en.wikipedia.org/wiki/Smooth_Operator

This is mostly a port of this video to RxJS https://www.youtube.com/watch?v=4TYv2PhG89A

Credit: I can't remember who first came up with this. May have been @benlesh. Let's just say it was all of us.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 97.612% when pulling e6b9d4f on jayphelps:smooth into 3e9d529 on ReactiveX:master.

@jayphelps jayphelps requested review from staltz and benlesh March 31, 2017 19:09
declare const expectObservable: typeof marbleTestingSignature.expectObservable;
declare const expectSubscriptions: typeof marbleTestingSignature.expectSubscriptions;

/** @test {smooth} */
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm the other tests have them. Not sure what it's for. figured it was some sort of build time thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's just say it's critical.

@staltz
Copy link
Member

staltz commented Mar 31, 2017

LGTM except for one detail.

@Boshen
Copy link
Contributor

Boshen commented Apr 1, 2017

image

@benlesh benlesh merged commit 07905e2 into ReactiveX:master Apr 1, 2017
@SteveALee
Copy link

Only for use in diamond shaped flows! :)

@nvartolomei
Copy link

Does not work in latest Safari :[

@martinmcwhorter
Copy link

I think there may be an inconsistency in the bridge where it states "coast to coast, LA to Chicago"... Which technically is in a coast... Oh nevermind...

@tetsuharuohzeki
Copy link
Contributor

I love software's easter egg and april fool joke. But why this operator is released as a part of default bundled operator?

This is not a code comment which can removed by some minifier and imported by default. If user uses import { Observable } from 'rxjs' because this operator is loaded by import './add/operator/smooth'; in src/Rx.ts.

This increases the file size of application code which uses rxjs by joke code. I strongly against to ship this operator with import './add/operator/smooth'; in src/Rx.ts.

@jayphelps
Copy link
Member Author

jayphelps commented Apr 3, 2017

Because it was only released in a custom April fools build. v5.2.1-smooth. 😝

@tetsuharuohzeki
Copy link
Contributor

@jayphelps

Because it was only released in a custom April fools build. v5.2.1-smooth. 😝

It is difficult to know that it's just a joke since it has been published in stable channel and has not been disabled yet in April 2nd (PST). 😫

jayphelps added a commit to jayphelps/rxjs that referenced this pull request Apr 3, 2017
@jayphelps jayphelps mentioned this pull request Apr 3, 2017
@jayphelps
Copy link
Member Author

jayphelps commented Apr 3, 2017

@saneyuki Perhaps you can clarify? It wasn't published as 5.2.1 it was published as 5.2.1-smooth. Someone would have to knowingly install that, and if they don't realize it's a joke there's really not much harm done.

It's going to be removed from master shortly, if that's what you mean--but just cause code is in master doesn't make it stable. 😄 It was all in good fun I don't think we broke anyone! 🎉

@pelotom
Copy link

pelotom commented Apr 3, 2017

rxjs@5.2.1-smooth currently shows up as the latest version in NPM, i.e. it's what you get by default when you npm install rxjs. I trust the joke doesn't introduce breaking changes, but it's not very professional to release joke code that gets marked as your latest stable release.

@martinmcwhorter
Copy link

martinmcwhorter commented Apr 3, 2017 via email

@pelotom
Copy link

pelotom commented Apr 3, 2017

You would have had to explicitly chosen to install the specific version

@martinmcwhorter no, you don't have to explicitly choose that version to get it. If you type npm install rxjs you will get it. Again, it's not a huge deal, but it's not great either.

@pelotom
Copy link

pelotom commented Apr 3, 2017

I see latest has now been bumped to 5.3.0, thanks! 👍

@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.

10 participants