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(ofType): expose ofType as lettable operator #343

Merged
merged 1 commit into from Oct 26, 2017

Conversation

@jgoz
Copy link
Contributor

@jgoz jgoz commented Oct 25, 2017

Supports using ofType directly in lettable operator pipelines.

Closes #186

Supports using ofType directly in lettable operator pipelines.

Closes #186
Copy link
Member

@jayphelps jayphelps left a comment

AWESOME!! Thank you @jgoz! Nothing to fix, it looks spot on!

@jayphelps jayphelps merged commit fb4a5af into redux-observable:master Oct 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jgoz jgoz deleted the jgoz:oftype-operator branch Oct 26, 2017
@jgoz
Copy link
Contributor Author

@jgoz jgoz commented Oct 26, 2017

Happy to contribute! Thanks for the quick turnaround 😃

@rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Oct 27, 2017

Thank you so much!

@BerkeleyTrue
Copy link
Collaborator

@BerkeleyTrue BerkeleyTrue commented Oct 30, 2017

@jayphelps when will this be released?

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

@BerkeleyTrue there ended up being an issue with it with versions of rxjs pre-lettable. I'm still debugging.

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

I think the issue is actually not version, but that the global version of rxjs doesn't provide let as letProto, it provides it as letBind. I dunno, shit is confusing.

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

That is indeed the issue. The named export is called letProto but the prototype name is letBind, so webpack-rxjs-externals doesn't correctly resolve it for UMD. Tracking here: ReactiveX/rxjs#3024

Not sure of how to work around..even if a new rxjs version fixes this, we wouldn't be able to support older versions of rxjs which is a deal breaker IMO. Anyone have ideas?

@jgoz
Copy link
Contributor Author

@jgoz jgoz commented Oct 30, 2017

@jayphelps Hmm, I wasn't aware of webpack-rxjs-externals. We can't go back in time and rename letProto to letBind, so maybe the simplest solution is to copy the implementation back into ActionsObservable.ofType instead of using the operator directly.

Other options:

  • Inside ActionsObservable.js do import 'rxjs/add/operator/let' and use let/letBind directly (downside being it pollutes Observable for users, but let is super tiny...)
  • Expose ofType as a bindable operator too (ofTypeProto? using the lettable version internally) and bind to that in ActionsObservable instead of letProto
@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

lol what I am thinking...letProto is so simple we don't even need to use it. 😄

return ofType(...keys)(this)

I'll make this change.

@jgoz
Copy link
Contributor Author

@jgoz jgoz commented Oct 30, 2017

Haha, that is by far the best option and I wish I'd thought of it initially 😉

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 30, 2017

#345 fixes that issue. I also want to get #346 reviewed and merged before we release the next version. Reviews from anyone are welcome!

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Oct 31, 2017

Released in v0.17.0 🎈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants