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.

Sign up for GitHub

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

Jump to bottom

Fix recursion in sync routine. #13818

Merged
merged 5 commits into from
Feb 12, 2019
Merged

Fix recursion in sync routine. #13818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2e1572
yield instead
nerrad Feb 11, 2019
6c1f236
add changelog entry
nerrad Feb 11, 2019
d3e6b50
woops fix incorrect link
nerrad Feb 11, 2019
ed960d8
add unit test reproducing bug and demonstrating fix
nerrad Feb 11, 2019
06ceb65
improve test description
nerrad Feb 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/redux-routine/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

- Fix unhandled promise rejection error caused by returning null from registered generator ([#13314](https://github.com/WordPress/gutenberg/pull/13314))
- The middleware will no longer attempt to coerce an error to an instance of `Error`, and instead passes through the thrown value directly. This resolves issues where an `Error` would be thrown when the underlying values were not of type `Error` or `string` (e.g. a thrown object) and the message would end up not being useful (e.g. `[Object object]`).
([#13315](https://github.com/WordPress/gutenberg/pull/13315))
([#13315](https://github.com/WordPress/gutenberg/pull/13315))
- Fix unintended recursion when invoking sync routine ([#13818](https://github.com/WordPress/gutenberg/pull/13818))

## 3.0.3 (2018-10-19)

Expand Down
2 changes: 1 addition & 1 deletion packages/redux-routine/src/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export default function createRuntime( controls = {}, dispatch ) {
// Async control routine awaits resolution.
routine.then( yieldNext, yieldError );
} else {
next( routine );
yieldNext( routine );
}
return true;
} );
Expand Down
17 changes: 17 additions & 0 deletions packages/redux-routine/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,21 @@ describe( 'createMiddleware', () => {

expect( store.getState() ).toBe( 2 );
} );

it( 'does not recurse when action like object returns from a sync ' +
'control', () => {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

image

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.

Copy link
Contributor Author

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.

const post = { type: 'post' };
const middleware = createMiddleware( {
UPDATE: () => post,
} );
const store = createStoreWithMiddleware( middleware );
function* getPostAction() {
const nextState = yield { type: 'UPDATE' };
return { type: 'CHANGE', nextState };
}

store.dispatch( getPostAction() );

expect( store.getState() ).toEqual( post );
} );
} );