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

Wrap actions from unbatched lists in the tagger from Cmd.map #235

Merged
merged 1 commit into from
Apr 19, 2020

Conversation

bdwain
Copy link
Member

@bdwain bdwain commented Mar 25, 2020

@laszlopandy this should address the list issue from #218

@@ -82,7 +84,7 @@ function handleParallelList(
}

return possiblePromise.then(result => {
return Promise.all(result.map(a => dispatch(a)));
return Promise.all(result.map(a => wrappedDispatch(a)));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key is here. We use the wrapped dispatch rather than the vanilla one.

I could see something like this maybe being needed in the new timeout/delay Cmd types also, but we can deal with that after this is merged (or I will add it if that gets merged first)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also realized this will be an issue for Cmd.setTimeout. And yes we can solve it separately.

src/cmd.js Outdated Show resolved Hide resolved
@laszlopandy
Copy link
Contributor

laszlopandy commented Apr 18, 2020

@bdwain I had another idea after reading the discussion of the return type of dispatch #237

What if instead of returning Promise<Array<Action>>, executeCmd returned Array<Promise<Action>>? In that way you could maintain the behaviour of parallel list execution (without batch) by returning a list of promises which resolve at different times. The passed dispatch argument would be used solely for run commands that request it with Cmd.dispatch.

The disadvantage is that this idea would require a bigger refactor. On the plus side it makes executeCmd conceptually simpler in that all actions would be returned. Whereas now some are returned (to be dispatched by the caller) and some are dispatched internally.

@bdwain
Copy link
Member Author

bdwain commented Apr 19, 2020

@laszlopandy can you make a separate issue to discuss that? It sounds interesting, though I'm not totally clear on what problem it's solving. But I think whatever it is would be separate from this change.

@bdwain
Copy link
Member Author

bdwain commented Apr 19, 2020

@laszlopandy one other thing is that that change sounds like it could potentially be a breaking change, and if so, we'll want to wait to do a v6 until we have it merged, since this and a few of the other changes going out are breaking too.

@bdwain bdwain merged commit f5be5d2 into master Apr 19, 2020
@bdwain bdwain deleted the fix-list-map-gotcha branch April 19, 2020 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants