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

Rewrite gatsby-link in typescript #22027

Closed
wants to merge 7 commits into from

Conversation

herecydev
Copy link
Contributor

@herecydev herecydev commented Mar 7, 2020

Description

Moves gatsby-link into the typescript world.

Related Issues

Related to #21995

TODO

@blainekasten can you provide some assistance, I have a few things I'm uncertain about:

  1. Should the index.d.ts be deleted? As I understand this should now be autogenerated, with the typescript files being the source of truth now.
  2. Everything is fairly trivial but the ref/innerRef is causing typescript some problems. There's some real confusion in this definitelytyped thread and the type signature is not easy to go through

image

  1. Should proptypes be in there still? If so, are they to be autogenerated? Or just a carbon copy of typescript props interface?
  2. Babel and lint had quite the fight, around L14 you can my efforts to disable eslint to get babel to transpile correctly

Happy to keep working on it but if you have any suggestions let me know

@herecydev herecydev requested a review from a team as a code owner March 7, 2020 09:06
handleRef(ref: HTMLAnchorElement | null): void {
if (
this.props.innerRef &&
Object.prototype.hasOwnProperty.call(this.props.innerRef, `current`)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blainekasten
Copy link
Contributor

  1. I think we should leave the index.d.ts for now, I need to look into this more
  2. The typings on the ref stuff looks fine. Is there a real problem here? sorry if I'm not following.
  3. Leave the proptypes in there. We can look into auto-generating them later if we want, but as a library it still provides value to some people.
  4. Shouldn't these just be defined on the window interface you have here?

@@ -173,48 +219,53 @@ class GatsbyLink extends React.Component {
// loaded before continuing.
navigate(to, { state, replace })
}

return 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.

Double checking this is an appropriate change. As I understand React doesn't respect any return values (typings also want a void method)

@herecydev
Copy link
Contributor Author

herecydev commented Mar 8, 2020

  1. I think we should leave the index.d.ts for now, I need to look into this more
  2. Leave the proptypes in there. We can look into auto-generating them later if we want, but as a library it still provides value to some people.

These are both restored, I've got some concerns about what the source of truth is. Proptypes, Typescript and the .d.ts all have a different understanding of the props. I can spend some time trying to converge them better, thoughts?

  1. The typings on the ref stuff looks fine. Is there a real problem here? sorry if I'm not following.

Babel will transpile happily but Typescript compiler is not happy. So the PR isn't mergeable until these are fixed

image

The signature looks something like: type Ref<T> = { bivarianceHack(instance: T | null): void }["bivarianceHack"] | RefObject<T> | null; so I'm not exactly sure how to fix this.

  1. Shouldn't these just be defined on the window interface you have here?

This would mean the code has to become window.__BASE_PATH__ and node won't have access to it.

@ascorbic
Copy link
Contributor

ascorbic commented Mar 8, 2020

We should absolutely aim to move to auto generating the typings. However at the moment we're compiling with Babel, so can't generate them as part of the compilation step. We'd need to either switch to compiling the package with tsc, or run it separately to generate the typings.

@blainekasten blainekasten self-assigned this Mar 9, 2020
@Eyas
Copy link
Contributor

Eyas commented Mar 25, 2020

I just saw this PR already attempts this. Sorry for duplicating this work in #22563

Note that I also remove index.d.ts but added a typings generation step to introduce a compatible one. Happy to work on this on either PR

@pragmaticpat pragmaticpat added the sprint-candidate To be considered for future core sprint label May 10, 2021
@LekoArts LekoArts added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed topic: TypeScript migration labels May 28, 2021
@imjoshin
Copy link
Contributor

Hey! Thanks so much for opening this pull request!

Sadly this PR got stale and didn't have any activity for some time. We're trying to do better with PR reviews! To get a better overview of all actionable PRs we're going through all open PRs and triage them. Since we won't be able to do everything and adding new features always means added maintenance burden, we have to be more picky about what's beneficial for the average user and the project itself longterm.

We think this is a great PR and would love to see it land in Gatsby. We're closing this PR for now but if you're able to rebase onto the latest master branch and try it out with the latest next versions, we'd be happy to review the PR when its re-opened.

We absolutely want to have you as a contributor and are sorry for any inconveniences we caused with replying too late to this PR.

Thanks for submitting to Gatsby! 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprint-candidate To be considered for future core sprint topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants