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

More complete types for RouterPush and abstact implementation into createRouterPush #59

Merged
merged 3 commits into from
Jan 21, 2024

Conversation

pleek91
Copy link
Contributor

@pleek91 pleek91 commented Jan 20, 2024

Description

Updates the types for RouterPush to be a little more accurate. Was hoping to remove the need for an as any but still had that same issue in createRouterPush. But at least now its consistent with createRouteMethods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but when I was testing some things to help with the recursive types I added this and felt it was worth keeping.

name: TRoute,
params?: TRoute extends keyof Flattened<TRoutes> ? Flattened<TRoutes>[TRoute] : Record<never, never>,
}
type RoutePushConfigParams<
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type just makes sure the params property is not required if there are no required params.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated but noticed this didn't get updated when I added query to the route types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated createRouteMethods to accept an object rather than multiple arguments after I wrote createRouterPush that way and thought it felt good. So this is the same functionally.

Comment on lines +47 to +48
function replace(url: string, options: RouterReplaceOptions = {}): Promise<void> {
return push(url, { ...options, replace: true })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated replace to just call router.push. That way theres only one implementation of what "replace" means.

import { component } from '@/utilities/testHelpers'
import { assembleUrl } from '@/utilities/urlAssembly'

type DummyRoutes = [{ name: string, path: '/', component: typeof component }]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what you think of this. I'm using this below in the push implementation on line 16 so that typescript knows the correct types for the arguments and return types. I cannot use just RouterPush<[]> because an empty array produces no type for the overload and then it says string is the only type for urlOrRouteConfig.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't use Route? Is this intended to be a temporary fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot use Route or Routes because they both have generics that cause the infinite depth errors. But something fixed that is more narrow works just fine.

Not intended to be temporary. Its gross. And open to other suggestions. But this is the most type safe I could make this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense. The things I really don't like about it is the name and the fact that it uses component from our utilities/testHelpers.

What if we changed the type to

type AnyRoute = [{ name: string, path: string, component: RouteComponent }]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that totally work. Good suggestion

@pleek91 pleek91 merged commit 58d52fe into main Jan 21, 2024
2 checks passed
@pleek91 pleek91 deleted the router-push-types branch January 21, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants