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): integration with redux-actions. #348
feat(ofType): integration with redux-actions. #348
Conversation
I'd also really appreciate support for toString implementations. If someone wants to detail the concern surrounding |
Hey @theengineear thanks for the PR! I think if we only applied toString when the type is a function, this will continue to work for Symbols too. Something like.. const key = keys[i];
if (key === type || (typeof key === 'function' && key.toString() === type)) {
} |
@jayphelps yup 👌 , more or less what I'm doing:
|
@theengineear hmm I'm not seeing that code. Did you forget to push perhaps? |
@jayphelps just locking down the spec. I realized that if the toString implementation of an actionCreator returns a Symbol, you will get an error converting a Symbol to a string. I'll ping you when it's up/done. |
da56a27
to
d8806b4
Compare
test/operators-spec.js
Outdated
expect(LARF_TYPE.toString()).to.equal(HAHA_TYPE.toString()); | ||
expect(String(HAHA_TYPE)).to.equal(String(LULZ_TYPE)); | ||
expect(String(LULZ_TYPE)).to.equal(String(LARF_TYPE)); | ||
expect(String(LARF_TYPE)).to.equal(String(HAHA_TYPE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised to see that String(Symbol())
doesn't throw an error. But...
const foo = () => {};
foo.toString = () => Symbol();
String(foo);
DOES throw an error. Is this something that I'm not understanding that I need to take into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString
should always return a string. If someone overrides it and does not return a string, that's their fault lol 😄
@jayphelps OK. All set. However, it might be a bad idea to allow a function's |
Some context. Looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except I don't think we need those sanity checks on Symbols themselves, only the tests against ofType
test/operators-spec.js
Outdated
const LARF_TYPE = Symbol(); | ||
|
||
// Sanity check. | ||
expect(HAHA_TYPE).not.to.equal(LULZ_TYPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious..why are these sanity checks needed? I don't think we need to test Symbols themselves in contexts unrelated to ofType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obv do let me know if you think they're needed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, ya I can take them out. I thought it might be helpful to document why the test is there, but I think it's clear from the test string.
The actionCreators you make with redux-actions implement their own toString, which means you don't have to keep track of action types at all. Before this commit, you need to: ``` ofType(myActionCreator.toString()) ``` In order to get this to work as expected. As of this commit, you can: ``` ofType(myActionCreator) ```
d8806b4
to
cad200c
Compare
@jayphelps ok, updated 🎉 |
So awesome. Thank you very much @theengineear!!! |
Released in v0.17.0 🎈 |
Thanks! I had some dangling comments in here, which I'll go ahead and post even though this is merged and we're good to go. Even though
Generally I'd say my statement on |
Thanks for this release @jayphelps, it's awesome. |
The actionCreators you make with redux-actions implement their own
toString, which means you don't have to keep track of action types at
all. Before this commit, you need to:
In order to get this to work as expected. As of this commit, you can:
Closes: #192