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

Type checking not enforced when passing strings instead of objects #64

Closed
malkhuzayyim opened this issue Jan 29, 2023 · 8 comments · Fixed by #65
Closed

Type checking not enforced when passing strings instead of objects #64

malkhuzayyim opened this issue Jan 29, 2023 · 8 comments · Fixed by #65
Labels
enhancement New feature or request
Milestone

Comments

@malkhuzayyim
Copy link

malkhuzayyim commented Jan 29, 2023

First of all, this is an awesome project, very straightforward to add, thank you for publishing it and sharing it.

I noticed something when using it that could be improved.

const router = useRouter();

router.push("/whatever") <-- this isn't type checked

I understand the type checking is only done when navigating using route "names" not "paths", via the object form of defining navigation. But is there a way to make this throw a type error to force the object notation at least?

To prevent accidentally using the wrong notation and force apply the type system.

All the best.

@victorgarciaesgi victorgarciaesgi added the enhancement New feature or request label Jan 29, 2023
@victorgarciaesgi
Copy link
Owner

victorgarciaesgi commented Jan 29, 2023

Hi @malkhuzayyim and thanks for your kind words!

Yeah it's actually a feature planned in the roadmap, you can check in the readme.

There's a lot of catchs with this feature. as being able to type check route with dynamic and optional params, or query params. It will be a challenge for sure

Also it need to recognize urls on <NuxtLink/> because the component allow external navigation.

Will keep you updated when I have a beta ready (haven't started yet)

@victorgarciaesgi
Copy link
Owner

And I could add an option to disable passing a string to the router or NuxtLink!

@victorgarciaesgi victorgarciaesgi added this to the v2.2.2 milestone Jan 30, 2023
@victorgarciaesgi
Copy link
Owner

@malkhuzayyim Also, should router.push({path: "whatever"}) be correct for you?

@malkhuzayyim
Copy link
Author

Hi @malkhuzayyim and thanks for your kind words!

Yeah it's actually a feature planned in the roadmap, you can check in the readme.

There's a lot of catchs with this feature. as being able to type check route with dynamic and optional params, or query params. It will be a challenge for sure

Also it need to recognize urls on <NuxtLink/> because the component allow external navigation.

Will keep you updated when I have a beta ready (haven't started yet)

Yeah I noticed the support for paths with string literals is on the roadmap. That'd be nice but I can imagine the complexity in doing that is somewhat significant.

And I could add an option to disable passing a string to the router or NuxtLink!

That is precisely what I was hoping would be available. It's probably a quick and easy solution to enforce router type safety for now.

The reason I think this is useful is, I used to be in the habit of passing strings, but now that I can have typesafety, I don't mind swapping exclusively to object notation, and this is a greenfield project, so it'd be nice to have a way to force object notations as a quick fix atleast, to prevent myself (or others in the codebase) from falling into old habits accidentally.

@malkhuzayyim Also, should router.push({path: "whatever"}) be correct for you?

I haven't used paths in object notations with the typed router yet. Historically I've only used paths like that if I'm passing a path and a query object. I swapped to using named routes with the typed router. I'd assume paths inside the object notation is about the same thing as passing strings directly to the push or NuxtLink component.

@victorgarciaesgi
Copy link
Owner

Yeah the path autocomplete will be in future release.

As I don't know what's the best way to do it, I will provide customisation options to the strict mode.
That way anyone can tweak the config the enable custom strictness in the RouteLocation.
It will stay false by default to avoid breaking apps.

If you could test it on v2.2.2-beta.0 that would be great! 😁
I still have to add tests and docs for the options part, but it's already working.

Here is the strict option interface

export interface ModuleOptions {
  //...
  /**
   * Customise Route location arguments strictness for `NuxtLink` or `router`
   * All strict options are disabled by default.
   * You can tweak options to add strict router navigation options.
   *
   * By passing `true` you can enable all of them
   *
   * @default false
   */
  strict?: boolean | StrictOptions;
}

export interface StrictOptions {
  NuxtLink: StrictParamsOptions;
  router: StrictParamsOptions;
}

export interface StrictParamsOptions {
  /**
   * Prevent passing string path to the RouteLocation argument.
   *
   * Ex:
   * ```vue
   * <template>
   *   <NuxtLink to='/login'/> // Error ❌
   * </template>
   * ```
   * Or
   * ```ts
   * router.push('/login'); // Error ❌
   * ```
   *
   * @default false
   */
  strictToArgument?: boolean;
  /**
   * Prevent passing a `params` property in the RouteLocation argument.
   *
   * Ex:
   * ```vue
   * <template>
   *   <NuxtLink :to='{path: "/login"}'/> // Error ❌
   * </template>
   * ```
   * Or
   * ```ts
   * router.push({path: "/login"}); // Error ❌
   * ```
   *
   * @default false
   */
  strictRouteLocation?: boolean;
}

@victorgarciaesgi
Copy link
Owner

Updated to v2.2.2-beta.1, fixed some options issues

@malkhuzayyim
Copy link
Author

Awesome, I'll give it a try tomorrow.

@malkhuzayyim
Copy link
Author

malkhuzayyim commented Feb 1, 2023

Updated to v2.2.2-beta.1, fixed some options issues

Tried this just now with strict: true, works exactly as I hoped it would. I'd suggest keeping this in the next release, its pretty neat. Thank you!

@victorgarciaesgi victorgarciaesgi linked a pull request Feb 1, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants