Initial typescript declarations and tests #144
Conversation
|
I’ve been on the road. Will look next week. |
src/index.d.ts
Outdated
| */ | ||
| type TransitionHook = ( | ||
| location: Location, | ||
| ) => undefined | (boolean | string | Promise<boolean | string>) |
There was a problem hiding this comment.
It's a matter of style, my local prettier formatted this so it is consistent
src/index.d.ts
Outdated
| [key: string]: any | ||
| } | ||
|
|
||
| function createBrowserRouter({ |
There was a problem hiding this comment.
does the spread/destructuring functionally do anything here? wouldn't be the same to do options: CreateBrowserRouterArgs?
There was a problem hiding this comment.
Not needed, it was when I copied in the signature. I'll remove
src/index.d.ts
Outdated
| // Improve these `any`s as needed | ||
| type FarceRouter = any | ||
|
|
||
| function createFarceRouter({ |
There was a problem hiding this comment.
another one here :) why not making a type out of these args and use that instead of any?
There was a problem hiding this comment.
As noted, this is a starting point. Feel free to enhance. I have typed everything I use (and more), now it's time for everyone to make it better.
|
@itajaja Do the definitions look good to you? |
|
definitions are 👍 The only thing is that the |
|
maybe adding the |
|
@itajaja |
|
@rosskevin I'm sorry for the delay here. We've been really busy lately. I'm going to look to try to merge this next week. |
src/index.d.ts
Outdated
| /** | ||
| * map version of search string | ||
| */ | ||
| query: Map |
There was a problem hiding this comment.
this is a little stricter – it's string -> string
src/index.d.ts
Outdated
| */ | ||
| interface Match { | ||
| location: Location | ||
| params: Map |
There was a problem hiding this comment.
this is a little stricter – it's string -> string (i think?)
src/index.d.ts
Outdated
| * Returns the path string for a pattern of the same format as a route path and a object of the | ||
| * corresponding path parameters | ||
| */ | ||
| format: (pattern: any, params: Map) => any |
There was a problem hiding this comment.
This will support non-string things in params though
src/index.d.ts
Outdated
| /** | ||
| * Lenient arg version of location using in #push and #replace. | ||
| */ | ||
| type LocationArg = Pick<Location, 'pathname'> & |
There was a problem hiding this comment.
the union of this and "string" below is referred to as a "location descriptor" in the other docs: https://github.com/4Catalyzer/farce#locations-and-location-descriptors
src/index.d.ts
Outdated
| * Navigates to a new location | ||
| * @see farce | ||
| */ | ||
| public push: (location: string | LocationArg) => void |
There was a problem hiding this comment.
can you define the arg here as a LocationDescriptor?
src/index.d.ts
Outdated
| * Navigates to a new location | ||
| * @see farce | ||
| */ | ||
| public push: (location: string | LocationArg) => void |
There was a problem hiding this comment.
this actually does return whatever the action dispatch returns, but i'm guessing you wouldn't want to use that return value
src/index.d.ts
Outdated
| */ | ||
| public go: (delta: number) => void | ||
|
|
||
| public createHref: Function |
There was a problem hiding this comment.
(location: LocationDescriptor) => string
src/index.d.ts
Outdated
| public go: (delta: number) => void | ||
|
|
||
| public createHref: Function | ||
| public createLocation: Function |
There was a problem hiding this comment.
(location: LocationDescriptor) => Location
src/index.d.ts
Outdated
| declare module 'found' { | ||
| import * as React from 'react' | ||
|
|
||
| interface Map { |
There was a problem hiding this comment.
is it standard to call this Map? could this be ambiguous w/ES6 Map?
|
Thanks for the review @taion - all look like good/appropriate changes. I'm underwater with some other efforts, I suggest merging and allowing other contributors to make it better. I'm pretty buried so feel free to do as you like, it's working for me for the last month so I don't have the impetus to pick it back up. |
|
Yeah, same here, sorry. We can pick this up and try to get this merged soon. |
|
Any news on this? |
|
@rosskevin Great work! |
|
Gah I totally forgot about this. |
|
I updated this PR to be mergeable again (including #174) and I addressed my own PR comments. Can someone here take a look? If I can get a quick sign-off here, I'd be happy to merge and cut a release. |
Closes #142
I have been using these definitions locally - so far so good.
Many of the types have been documented and are solid based on my use, though some of the others have been less traveled and are typed as
any. Whileanyis relevant in some situations, often it is bailing out. So these can be improved as we go.In
tests/typescriptI have added the sample usage from the readme, as well as added thetypescriptscript in for testing. Thistest-usage.tsxfile can be expanded, or other files added to the directory as holes are found.I think this setup should be fairly easy to keep up/validate. I expect others will find holes and PR small changes from here as well.