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

Refactor error with enum codes #123

Merged
merged 16 commits into from
Mar 18, 2020
Merged

Refactor error with enum codes #123

merged 16 commits into from
Mar 18, 2020

Conversation

fnlctrl
Copy link
Member

@fnlctrl fnlctrl commented Feb 29, 2020

No description provided.

@fnlctrl fnlctrl requested a review from posva February 29, 2020 08:23
@codecov-io
Copy link

codecov-io commented Mar 9, 2020

Codecov Report

Merging #123 into master will decrease coverage by 4.21%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   80.95%   76.73%   -4.22%     
==========================================
  Files          22       23       +1     
  Lines         924      950      +26     
  Branches      243      249       +6     
==========================================
- Hits          748      729      -19     
- Misses        141      185      +44     
- Partials       35       36       +1     
Impacted Files Coverage Δ
src/router.ts 78.03% <75.00%> (ø)
src/errors-new.ts 91.30% <91.30%> (ø)
src/matcher/index.ts 97.22% <100.00%> (+0.05%) ⬆️
src/utils/guardToPromiseFn.ts 100.00% <100.00%> (ø)
src/errors.ts 0.00% <0.00%> (-70.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dce7bc...827b859. Read the comment docs.

src/errors-new.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
to?: RouteLocation
}

const ErrorTypeMessages = {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need some kind of logError method, similar to vue-next. In our case error codes are part of the public api but I think the messages should only be logged in dev mode and just print the error in prod
At this point I wonder if message should just be kept empty 🤔

Copy link
Member Author

@fnlctrl fnlctrl Mar 17, 2020

Choose a reason for hiding this comment

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

I added a __DEV__ check to remove it in prod build. logError seems unnecessary as we're not catching all errors then print/rethrow them like in core, maybe?

src/errors-new.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
@fnlctrl fnlctrl requested a review from posva March 17, 2020 10:26
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.

This is starting to look great!
I'm still concerned about the names but we can change that after merging this. Although it's a great opportunity to get others involved:

  • Aborted / Cancelled are too similar and will lead to confusion. Do we even need 2 different kind of errors? Even when thinking about other names like dropped, it fits both cases:
    • next(false)
    • A newer push/replace being called while the navigation was ongoing

I think it's worth splitting up the error codes: Navigation Errors (exported and can be used by users) and others like the matcher error that are used internally and we want to (at least for the moment) keep private

src/errors-new.ts Outdated Show resolved Hide resolved
src/router.ts Outdated Show resolved Hide resolved
src/router.ts Outdated Show resolved Hide resolved
src/utils/guardToPromiseFn.ts Outdated Show resolved Hide resolved
src/errors-new.ts Outdated Show resolved Hide resolved
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.

Let's merge this and start a discussion regarding what navigation failures need to be exposed

@posva posva merged commit 44486bc into master Mar 18, 2020
@posva posva deleted the refactor-error branch March 18, 2020 16:04
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.

3 participants