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

feat(dx): warn when passing undefined/null locations #2158

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

skirtles-code
Copy link
Contributor

This PR attempts to improve the warnings and error handling for a common problem encountered during development.

Background

There's an error message I've encountered several times on Vue Land:

TypeError: Cannot use 'in' operator to search for 'path' in undefined

This comes from router.resolve(), e.g.:

That's a silly example, it's rarely quite so obvious when someone is having this problem in a real application.

These two issues are both hitting the same problem (the first one is in the wrong repo):

You get something similar with null instead of undefined.

router.push(undefined) gives the same error, as that uses router.resolve().

Most commonly this is hit via useLink(). Here's a simple example of that:

Again, I've made it obvious what the problem is with to: undefined, but real code is rarely so blatant.

It is possible to hit this same error via RouterLink, but in that case we get a prop validation warning, making it a bit easier to figure out what the problem is:

The mistake in that example is a typo in patn: '/about'.

But even with the prop validation warning, it can be difficult to identify exactly which component has the problem. If we could inspect the components using Vue Devtools it'd be much easier, but that isn't currently an option because rendering bombs out.

The Vuetify problem

Someone on Vue Land was hitting this error, but without any warnings to indicate what they were doing wrong. They weren't using RouterLink, they were using Vuetify's breadcrumb component.

The ultimate cause of the problem was an undefined in their data that they hadn't taken into account. But this isn't quite as simple as it sounds.

There's this problematic line in Vuetify:

The problem here is that props.to can be undefined. While the Vuetify composable does check that, it only checks the initial value. In the application we encountered on Vue Land, the to prop was populated initially, only becoming undefined after some new data loaded.

It isn't obvious to me how Vuetify could fix this problem in their own code. There doesn't seem to be a way to disable the useLink() composable when the prop is undefined and re-enable it when the value is defined.

This PR doesn't attempt to address that problem directly, it just aims to improve the warnings/errors to ease debugging.

What's changed

  • router.resolve() no longer throws an error in dev. It logs a warning instead. This allows Vue Devtools to be used to debug the problem.
  • useLink() logs warnings. This is usually a bit more helpful than the warning in router.resolve(), as it has more context to work with.
  • Vue Devtools will show an error label if props.to is invalid, making it much easier to find a rogue RouterLink in a long list:
    Vue Devtools

In cases where RouterLink has an undefined or null value for to, this will lead to 3 warnings being shown: one for prop validation, one for useLink() and one for router.resolve(). I couldn't see an easy way to avoid that, but I think that's much better than not having the warnings.

Copy link

netlify bot commented Mar 3, 2024

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit ff140d1
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/65e6a915ff44d90008ae6f74

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (13303bd) to head (ff140d1).
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2158      +/-   ##
==========================================
+ Coverage   90.90%   91.01%   +0.11%     
==========================================
  Files          24       24              
  Lines        1121     1135      +14     
  Branches      347      351       +4     
==========================================
+ Hits         1019     1033      +14     
  Misses         63       63              
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

I think this will be really useful! Thank you

packages/router/src/RouterLink.ts Show resolved Hide resolved
packages/router/src/RouterLink.ts Outdated Show resolved Hide resolved
packages/router/src/RouterLink.ts Outdated Show resolved Hide resolved
packages/router/src/devtools.ts Show resolved Hide resolved
packages/router/src/utils/index.ts Outdated Show resolved Hide resolved
packages/router/__tests__/router.spec.ts Outdated Show resolved Hide resolved
packages/router/__tests__/router.spec.ts Outdated Show resolved Hide resolved
packages/router/__tests__/useLink.spec.ts Outdated Show resolved Hide resolved
skirtles-code and others added 3 commits March 3, 2024 17:43
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
@skirtles-code skirtles-code requested a review from posva March 3, 2024 19:03
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.

Thanks!

@posva posva merged commit 089378b into vuejs:main Mar 5, 2024
5 checks passed
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