vue-router icon indicating copy to clipboard operation
vue-router copied to clipboard

The type definition of Route seems to be partially wrong

Open dora-gt opened this issue 3 years ago • 4 comments

Version

3.5.1

Reproduction link

https://github.com/vuejs/vue-router/blob/dev/types/router.d.ts#L203

Steps to reproduce

None, just the source code seems to be wrong.

What is expected?

The type definition of Route#query should be

query: Dictionary<string | null | (string | null)[]>

The current implementation cannot express this type of query

/path?foo#bar

as it doesn't have any value and it is not converted to an array.

What is actually happening?

https://github.com/vuejs/vue-router/blob/dev/types/router.d.ts#L203

The type definition of Route#query is

query: Dictionary<string | (string | null)[]>

dora-gt avatar Jun 09 '21 15:06 dora-gt

#3647

@posva Hello 👋 I'd like to add some information/context to this. Looking at the implementation of the stringifyQuery function I can see that it uses a function encode for primitive values other than undefined and null. This encode function internally uses encodeURIComponent on the value passed in.

Since encodeURIComponent accepts numbers and booleans as well, what do you think about allowing these as well in the typing for Location["query"]? The implementation already handles these as well, and the end result is the same, a string URL.

Example

vm.$router.push({
  name: 'someRoute',
  query: { foo: 'foo', bar: 1, baz: true, qux: ['hello', 1, false] },
});
// Type 'number' is not assignable to type 'string'.
// Type 'boolean' is not assignable to type 'string'.
// Type '(string | number | false)[]' is not assignable to type 'string'.

The result is: foo=foo&bar=1&baz=true&qux=hello&qux=1&qux=false.

If you agree with this I would gladly work on a PR (add tests as well). This should cover most use cases without any changes to the current functionality.

mussinbenarbia avatar Nov 16 '22 14:11 mussinbenarbia

@mussinbenarbia You can give it a try if you want to accept null in query in Route.

The reason number and false are not assignable when doing a push is because there isn't any casting (like in v4) so the final type of the query ends up staying a number (non consistent if you reload the page). Therefore, without adding the forced casting (which could break a lot of users code and that's why it was added in vue router 4 only), it's better to keep string and null as the only acceptable types.

posva avatar Nov 18 '22 07:11 posva

Thank you for the feedback 👍 I'll give it a go.

Still, I'd like if possible to clarify something. I understand that Route["query"], can only accept string or null, that's how vm.$route.query is typed and since we do not do any casting, no matter what we push to, the parsed values will always be strings.

Isn't it still possible to allow number and boolean in Location["query"] at push time? The way I'm intepreting it is, when we push to a Location, we can pass in an object in query the values of which can be string, number, boolean, null, undefined or an array of these types. When stringified, everything will be turned into a string (with the exception of particular handling done for undefined and null), and we will not get back the same types when we parse the query for example on page load.

Isn't that still an improvement? With the full understanding that no matter how Location["query"] is typed, Route["query"] at parse time will always be a Dictionary<string>. This still allows us to push without getting a TS error. Am I missing something?

Thank you!

mussinbenarbia avatar Nov 19 '22 01:11 mussinbenarbia