svelte-routing icon indicating copy to clipboard operation
svelte-routing copied to clipboard

Types in `useHistory` and `useRouter` are incorrect

Open Curstantine opened this issue 1 year ago • 1 comments

The contexts.d.ts file defines useHistory, useRouter as readable when they are objects with functions attached to them.

More context: Since these don't have a subscribe method attached to it, you can't use these functions with svelte's subscribe syntax. And the messy types are annoying to work with.

image

Curstantine avatar Feb 04 '24 15:02 Curstantine

Please raise a PR if you have better option. Thanks.

krishnaTORQUE avatar Feb 06 '24 09:02 krishnaTORQUE

@krishnaTORQUE

They are indeed wrong:

Test

const router = useRouter();
$: console.log(router)

Output

{activeRoute: {…}, base: {…}, routerBase: {…}, registerRoute: ƒ, unregisterRoute: ƒ}

What types say

type ROUTER = RouterProps;
export const useRouter: () => ReturnType<typeof readable<ROUTER>>;

type RouterProps = {
    basepath?: string;
    url?: string;
    viewtransition?: (direction?: string) => Viewtransition;
};

It's not really that the given option is not enough, it simply is incorrect.

RouterProps is the proper type for the props of <Router ...> component but not the useRouter

james-em avatar Feb 09 '24 14:02 james-em

@krishnaTORQUE

PR: https://github.com/EmilTholin/svelte-routing/pull/281

james-em avatar Feb 09 '24 15:02 james-em

@krishnaTORQUE

Thanks for merging. Hopefully it will be released soon! :)

I forgot to mention one thing

registerRoute: (route: Omit<RouteConfig, 'default'>) => {};
unregisterRouter: () => {}

registerRoute is properly typed but not unregisterRouter. However, there is an issue with it because internally it compares using === for exactly same route passed to registerRoute but the route is re-initialized by registerRoute internally making it impossible to have a working unregisterRouter

It would require some internal changes to do something like this:

const myRoute = ...
const registeredRoute = registerRoute(myRoute); // Currently returns nothing

// Let's say it would return something
myRoute === registeredRoute
=> false

unregisterRouter(myRoute) // Would fail
unregisterRouter(registeredRoute) // Would work

Just though I should let you know

james-em avatar Feb 09 '24 17:02 james-em

@krishnaTORQUE

Thanks for merging. Hopefully it will be released soon! :)

I forgot to mention one thing

registerRoute: (route: Omit<RouteConfig, 'default'>) => {};
unregisterRouter: () => {}

registerRoute is properly typed but not unregisterRouter. However, there is an issue with it because internally it compares using === for exactly same route passed to registerRoute but the route is re-initialized by registerRoute internally making it impossible to have a working unregisterRouter

It would require some internal changes to do something like this:

const myRoute = ...
const registeredRoute = registerRoute(myRoute); // Currently returns nothing

// Let's say it would return something
myRoute === registeredRoute
=> false

unregisterRouter(myRoute) // Would fail
unregisterRouter(registeredRoute) // Would work

Thanks for the PR. If there is any other changes require, you can raise PR for that as well.

krishnaTORQUE avatar Feb 09 '24 17:02 krishnaTORQUE

Hi @james-em Waiting for your response.

krishnaTORQUE avatar Feb 10 '24 07:02 krishnaTORQUE

Hi @james-em Waiting for your response.

Thanks for getting back. I do not need to call registerRoute or unregisterRoute for my usage. Just though I should let you know they are not usuable for now outside of the "expected" usage described in the README.

james-em avatar Feb 11 '24 19:02 james-em