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

Warn When dispatching During Middleware Setup #1345

Closed
wants to merge 2 commits into from

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Feb 1, 2016

Closes #1240

Any thoughts on the wording that was chosen? I assume we want to just warn and not throw a hard error here.

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

Can we absolutely not make it work? If we apply middleware from the innermost to the outermost, it seems like we could gradually update the outer dispatch reference as we fold them, can we not?

@timdorr
Copy link
Member Author

timdorr commented Feb 1, 2016

@yelouafi has a potential option here: #1240 (comment)

I'm not married to this PR, so I'd be more than happy to close it if we can find a solution. This just exists to make our job easier if we don't find one.

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

I guess what’s happening here is I don’t understand the current semantics without a test ;-)

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

To clarify, I see that you added a warning, and you test for it. But I want to understand what actually happens if you have several middlewares that attempt to dispatch actions in definition phase.

@timdorr
Copy link
Member Author

timdorr commented Feb 1, 2016

OK, I can work up something more elaborate.

@gaearon
Copy link
Contributor

gaearon commented Feb 1, 2016

I think we should just throw and call it a day. Warning will be frustrating because user can't control it, and middleware author will have to remove it anyway due to complaints.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2016

I’ll close because I’d like to see a hard error here, not a warning, and the PR has been inactive for a while. If you update this please reopen; otherwise somebody else might give it a shot if they find the time. Thanks!

@gaearon gaearon closed this Mar 6, 2016
@timdorr
Copy link
Member Author

timdorr commented Mar 6, 2016

Sorry, I was meaning to get back to this but keep forgetting. I'll try to get it clean up in the next couple days.

@gaearon
Copy link
Contributor

gaearon commented Mar 6, 2016

Thanks, no trouble. I’m just cleaning up non-actionable PRs so new contributors aren’t confused.

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.

2 participants