Skip to content

Conversation

fnlctrl
Copy link
Member

@fnlctrl fnlctrl commented Sep 1, 2016

Fix hash comparison for isSameRoute (#635)
when one of a.hash and b.hash is undefined while the other is empty string

This bug causes active class not being applied when using named view + exact matching

when one of a.hash and b.hash is undefined while the other is empty string

This bug causes active class not being applied when using named view + exact matching
@fnlctrl fnlctrl changed the title Fix hash comparison for isSameRoute (#635) Fix hash comparison for isSameRoute (fix #635) Sep 1, 2016
return (
a.path === b.path &&
a.hash === b.hash &&
(a.hash || '') === (b.hash || '') &&
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit hacky to be done here. Can't route get normalised somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.. Yes it's hacky.. I'll look deeper and see if I can normalize it elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

By definition, isSameRoute's parameter a and b is type Route, whose hash is a required string field, but sometimes b.hash is undefined. So there may be some bug flow wasn't able to discover.

Copy link
Member

@posva posva Sep 3, 2016

Choose a reason for hiding this comment

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

Actually, it's is required so the check has to be done somewhere

Copy link
Member Author

@fnlctrl fnlctrl Sep 3, 2016

Choose a reason for hiding this comment

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

@posva I think I've found the cause: It's in router-link's render function, where it passed a Location object (whose hash is optional) directly into isSameRoute:
see https://github.com/vuejs/vue-router/blob/next/src/components/link.js#L31-L33

I'm surprised that flow didn't warn on implicit type conversion: passing a Location object to isSameRoute, while it asked for a Route

Copy link
Member Author

Choose a reason for hiding this comment

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

To sum up, @yyx990803 A more elegant fix for this (instead of the hash || '' fix) involves modifying Location type, or a converter from Location to Route:

  1. compareTarget can be a Location
    https://github.com/vuejs/vue-router/blob/next/src/components/link.js#L31
  2. and it's passed into isSameRoute whose second param is type ?Route (which means Route | void | null if I understood correctly), but flow didn't give a warning
    https://github.com/vuejs/vue-router/blob/next/src/components/link.js#L33

Copy link
Member

Choose a reason for hiding this comment

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

Location can be an empty object because everything is optional in it.
Is there a way on flow to say a Location must have either a name or a path?
I think using multiple types and then saying location is one of them can work

Copy link
Member Author

@fnlctrl fnlctrl Sep 3, 2016

Choose a reason for hiding this comment

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

@posva A naive approach can be

declare type Location = {
  _normalized?: boolean;
  name?: string;
  path: string;
  hash?: string;
  query?: StringHash;
  params?: StringHash;
} | {
  _normalized?: boolean;
  name: string;
  path?: string;
  hash?: string;
  query?: StringHash;
  params?: StringHash;
}

But it seems clumsy... I'll dig up flow's documents

Copy link
Member

@posva posva Sep 3, 2016

Choose a reason for hiding this comment

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

I think we are looking for this https://flowtype.org/docs/disjoint-unions.html#_

edit: which is actually what you did xD

It'd be great to combine unions with intersections

Copy link
Member Author

Choose a reason for hiding this comment

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

@posva Tried that, but flow still doesn't give warnings : (
and even if we make it give warnings, we still have to fix passing Location to a function that requires Route..

@LinusBorg LinusBorg added the 2.x label Sep 5, 2016
@yyx990803
Copy link
Member

The root cause was because components/link was not flow-enabled... I've fixed it and made some adjustments in 270683f

@yyx990803 yyx990803 closed this Sep 13, 2016
@yyx990803 yyx990803 deleted the fix-hash-comparison branch September 19, 2016 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants