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

Handle Promise rejection in guards (fix #2833) #2838

Closed
wants to merge 12 commits into from

Conversation

oleksiikhr
Copy link

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR!
However, we don't use babel to transpile the lib, so this won't work (it is used with tests). Could you find a version without adding the babel plugin dependency?
Can you also add the tests for the others navigation guards, so they are all covered? beforeEnter, beforeRouteEnter, beforeRouteUpdate
Also, could you please add back the template for the PR? It helps us to organise things

Thanks

@oleksiikhr
Copy link
Author

Very interesting, solved without a library :)

Also, could you please add back the template for the PR? It helps us to organise things

What is a template?

@oleksiikhr oleksiikhr closed this Jul 2, 2019
@posva
Copy link
Member

posva commented Jul 3, 2019 via email

@oleksiikhr
Copy link
Author

I studied this code, found 2 places (hook) where Promise can be accepted (beforeEach, afterEach):

https://github.com/vuejs/vue-router/pull/2838/files#diff-feafce5e816936212eccd6808c5421a6R123
https://github.com/vuejs/vue-router/pull/2838/files#diff-feafce5e816936212eccd6808c5421a6R185

And if in the first case the execution of the code, the sequence itself does not change, in the second case it is not so (R185).

If we take into account that everything happens through callbacks, then in the function confirmTransition, I also need to transfer to onComplete the callback also the this._abort function. In the abstract.js, base.js files, receive and transmit the updateRoute function.

In general, I do not like it, I understand that most likely I need to wrap parts of the code in Promise, do consistent error handling and not break the original logic.

Since this PR has errors in logic, I decided to close it, so that suddenly due to inactivity, they would not begin to accept it :)

@posva
Copy link
Member

posva commented Jul 3, 2019

Thanks for the explanation. I wanted to make sure it wasn't an accident :)
With current codebase it's a bit harder to implement indeed

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