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

Moving ofType into a separate operator #186

Closed
wants to merge 1 commit into from

Conversation

@jshthornton
Copy link

@jshthornton jshthornton commented Feb 3, 2017

#185

Copy link
Member

@jayphelps jayphelps left a comment

Thanks for your PR! Commit message doesn't follow the conventional-changelog-standard format.

@@ -15,27 +16,12 @@ export class ActionsObservable extends Observable {
constructor(actionsSubject) {
super();
this.source = actionsSubject;
this.ofType = ofType.bind(this);

This comment has been minimized.

@jayphelps

jayphelps Feb 5, 2017
Member

What do you think about ActionsObservable.prototype.ofType = ofType; below the class instead?

@jayphelps
Copy link
Member

@jayphelps jayphelps commented Feb 5, 2017

How do you plan to import ofType? I don't believe this PR exposes a public API for just the function itself? It would need to be added to the index.js. Importing files from redux-observable/lib/ofType.js is not an officially supported, stable API--file names and locations may change without warning.

If you wanna add that index.js, that works 👍

@BerkeleyTrue
Copy link
Collaborator

@BerkeleyTrue BerkeleyTrue commented Apr 19, 2017

@jshthornton Are you still interested in completing this PR? I may have use for importing ofType on it's own.

jgoz added a commit to jgoz/redux-observable that referenced this pull request Oct 25, 2017
Supports using ofType directly in lettable operator pipelines.

Closes redux-observable#186
jayphelps added a commit that referenced this pull request Oct 26, 2017
Supports using ofType directly in lettable operator pipelines.

Closes #186
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

3 participants