-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Add typings for 'to' property of Gatsby Link #3552
Conversation
Deploy preview for gatsbygram ready! Built with commit 65cf341 |
Perhaps just make the type |
Could simply do type any. Looking at react router revealed custom type that I didnt dig too much into. The string part came to as I was reading samples on how to use Gatsby link. Nearly all of them had either strings or props that is a string. |
Yeah let's just do any. Most of the time you do only string but there are more advanced use cases where the object form is helpful. |
@KyleAMathews done, switched to any. |
Sweet! Another quick question — does the semicolon matter at all in type files? |
Originally I removed that explicit type because I incorrectly assumed it would inherit the to prop from |
@nsimonson that sounds pretty reasonable — how would we do that? |
@KyleAMathews in the interface, semicolons are not necessary at all. |
@nsimonson I am assuming then you would do smth like this
|
@Nikki1993 want to make the change to use locationDescriptor? |
Normally it's not necessary, it extends And // node_modules\@types\react-router-dom\index.d.ts
export interface LinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
to: H.LocationDescriptor;
replace?: boolean;
}
export class Link extends React.Component<LinkProps, any> {}
export interface NavLinkProps extends LinkProps {
activeClassName?: string;
activeStyle?: React.CSSProperties;
exact?: boolean;
strict?: boolean;
isActive?<P>(match: match<P>, location: H.Location): boolean;
location?: H.Location; |
And for the semicolumns I don't care, we just need to be consistent, actually we have some other typescript files with semicolumns (https://github.com/gatsbyjs/gatsby/blob/master/examples/using-typescript/src/html.tsx for example) so if you want to change the convention, I think that a real PR for this, (with all the typescript files linted with the same linter rules) should be better 😉 |
@KyleAMathews will do, was without a laptop so PR update inc @fabien0102 well if the type existed I wouldn't have made the PR 😄 I thought it was odd that 'to' property was missing but that "hack" with LocationDescriptor makes the original error in here #3525 (comment) go away so go figure 😕 |
@Nikki1993 It's very strange, I have used this gatsby link in my starter and the Can you try to check on your |
@fabien0102 what the... I have a feeling I might drop yarn off the balcony soon. I reinstalled everything with simple I am gonna go on a witch hunt and say yarn has been quite unreliable lately, especially when it comes to typings and definitions, especially with reactjs + typescript combo. Gonna close this as there is no problem with typings but most likely with package manager messing up the types. |
I see that |
This should fix #3525 (comment)
Based on documentation 'to' property always takes a string. I also prettified the file to be more consistent as certain lines had semicolons, some didn't etc.