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

[TypeScript] LocaleRoute should return Location #776

Closed
WTDuck opened this issue Jun 27, 2020 · 7 comments · Fixed by #777 or #1142
Closed

[TypeScript] LocaleRoute should return Location #776

WTDuck opened this issue Jun 27, 2020 · 7 comments · Fixed by #777 or #1142
Labels

Comments

@WTDuck
Copy link

WTDuck commented Jun 27, 2020

Version

nuxt-i18n: 6.12.2
nuxt: 2.13.1

What is the problem ?

From vue-router type definitions:

  • push(location: RawLocation): Promise<Route>
  • replace(location: RawLocation): Promise<Route>

Those methods expect type RawLocation = string | Location but localeRoute from nuxt-i18n returns Route object which is slightly different from Location

Solution expected

  • localeRoute(route: RawLocation, locale?: string): Route | undefined
  • should become localeRoute(route: RawLocation, locale?: string): Location | undefined
@WTDuck
Copy link
Author

WTDuck commented Jun 27, 2020

nuxt-i18n doc is referring to localeRoute along side with $router.push which will return a type error

@rchl
Copy link
Collaborator

rchl commented Jun 27, 2020

For reference: https://github.com/vuejs/vue-router/blob/f0d9c2d93cf06b5b2202bd77df6281cf47970b20/types/router.d.ts#L138-L158

I suppose it should work to just change the type. The object will still technically be a route and will potentially contain some extra properties but that should hopefully not cause any issues.

rchl added a commit that referenced this issue Jun 27, 2020
Event though the result is actually a route object, the point of
localeRoute is to return something that can be passed to VueRouter.push()
and that needs to be a Location object.

Location is basically a subset of Route plus some optional properties
so treating Route as Location should be fine.

Resolves #776
@rchl rchl closed this as completed in #777 Jun 27, 2020
rchl added a commit that referenced this issue Jun 27, 2020
Event though the result is actually a route object, the point of
localeRoute is to return something that can be passed to VueRouter.push()
and that needs to be a Location object.

Location is basically a subset of Route plus some optional properties
so treating Route as Location should be fine.

Resolves #776
@gimyboya
Copy link

gimyboya commented Apr 5, 2021

@rchl this issue is still relevant as the types have not been changed in the types declaration here

@rchl
Copy link
Collaborator

rchl commented Apr 5, 2021

That got accidentally reverted during types refactoring.

But now I'm having seconds thoughts about whether I should change it. The method is called localeRoute and it does return a Route object. The issue seems more to be in the example itself.

But at the same time, it does make it inconvenient to use with router.$push when it's a Route so not sure what should be done about that. Could possibly provide a helper function to transform one to the other, maybe.

@gimyboya
Copy link

gimyboya commented Apr 7, 2021

@rchl I am not sure what are all the use cases for this function but for me so far I only use it primarily with $router.push() and $router.replace() which in this case both expect a RawLocation type and I get a type error. However, if there are other use cases that require this function to return a Route object the I guess something like this would work:

localeRoute('myroute') // returns a Route
localeRoute('myroute').toRawLocation() // return a RawLocation

Currently, I am creating my own types and importing them:
types/Location.ts:

// types taken from vue-router types https://github.com/vuejs/vue-router/blob/dev/types/router.d.ts#L188
// this is needed until this is fixed https://github.com/nuxt-community/i18n-module/issues/776#issuecomment-813201177
type Dictionary<T> = { [key: string]: T };

export interface Location {
  name?: string;
  path?: string;
  hash?: string;
  query?: Dictionary<string | (string | null)[] | null | undefined>;
  params?: Dictionary<string>;
  append?: boolean;
  replace?: boolean;
}

And use as such:

this.$router.replace(this.localeRoute('thankyou') as Location);

@rchl
Copy link
Collaborator

rchl commented Apr 8, 2021

Changing the return type to Location. I think the primary use case is to use it with router.push but then it's probably a bit badly named. But too late to change that now.

@rchl
Copy link
Collaborator

rchl commented Apr 9, 2021

Changed my mind. Leaving localeRoute behavior unchanged and return type changed to reflect the reality (Route), and added localeLocation API that returns Location.

@rchl rchl closed this as completed in #1142 Apr 9, 2021
rchl added a commit that referenced this issue Apr 9, 2021
The intention of `localeRoute` was that it's used within `$route.push` call. The issue
with that was that `localeRoute` returned `Route` while `push` expects `Location`.

Changing the return type to `Location` while returning the route object would be wrong
so instead added a new API that returns `Location` and left `localeRoute` to return `Route`.

Fixes #776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants