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

The type definition of Route seems to be partially wrong #3566

Open
dora-gt opened this issue Jun 9, 2021 · 4 comments
Open

The type definition of Route seems to be partially wrong #3566

dora-gt opened this issue Jun 9, 2021 · 4 comments
Labels
contribution welcome fixed on 4.x This issue has been already fixed on the v4 but exists in v3 good first issue Typescript Typescript related issues

Comments

@dora-gt
Copy link

dora-gt commented Jun 9, 2021

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)[]>
izabela-grzeskowiak-allegro added a commit to izabela-grzeskowiak-allegro/vue-router that referenced this issue Oct 15, 2021
@izabela-grzeskowiak-allegro

#3647

@mussinbenarbia
Copy link

@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.

@posva
Copy link
Member

posva commented Nov 18, 2022

@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 posva added the fixed on 4.x This issue has been already fixed on the v4 but exists in v3 label Nov 18, 2022
@mussinbenarbia
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome fixed on 4.x This issue has been already fixed on the v4 but exists in v3 good first issue Typescript Typescript related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants