-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 recursion in sync routine. #13818
Conversation
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.
It would be good to add a unit test 🤷♂️
Hmm ya, I guess it would :) lemme see if I can write one that catches the bug. |
Added a unit test that fails without the fix in this branch and passes with the fix in this branch. |
@@ -143,4 +143,21 @@ describe( 'createMiddleware', () => { | |||
|
|||
expect( store.getState() ).toBe( 2 ); | |||
} ); | |||
|
|||
it( 'does not recurse when action like object returns from a sync ' + | |||
'control', () => { |
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.
Is the concatenation here something automated? Or are you especially strict with line length? 😄
I don't have any strong feelings on it, but it made it slightly more difficult for me to read the test case (or at least, it came as a surprise to me in reading the label).
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.
Strict with line length. It's a habit I've gotten into in other projects I've worked on but I'm happy to change if its an issue.
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.
Of the various options I've played around with for handling line length issues, this is the one I've landed on as the one I can be most consistent with and also indents nicely.
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.
Oh, I can appreciate it. I'm of a similar obsession, particularly for comments, even to the point of enabling the visual "line" for my editor.
I didn't intend to make a thing of it. My only worry might be that the indentation implies that it's of a similar level as the body of the callback, when in fact it's still part of the earlier (it
) level.
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'm of a similar obsession, particularly for comments, even to the point of enabling the visual "line" for my editor.
Heh! I do the same in my IDE + have my IDE configured to show me when line length is exceeded.
Description
While working on #13716 I discovered a bug where when a control returns the results with an object that has a
type
property,redux-routine
was recursing into that returned object treating it as an action.When I brought this up in slack, @youknowriad suggested it could be a bug in
redux-routine
and I tried his suggested fix and that seemed to work so this is the pull applying it.How has this been tested?
Types of changes
As far as I can tell, this is a non-breaking change.
Checklist: