chore(gatsby): convert gatsby-link to typescript #37543

Kornil · 2023-01-28T00:56:22Z

Description

Convert gatsby-link to TS.

Documentation

Related Issues

The previous PR was written 3 years ago and closed more than 6 months ago, as suggested in the closing message, I took the liberty of redoing it and open a new PR #22027

[Umbrella] TypeScript Migration (Help wanted!) #21995

Kornil · 2023-01-28T00:57:55Z

I do have a pesky ref type issue in packages/gatsby-link/src/index.tsx line 129 that I'd gladly accept suggestions on how to fix.

LekoArts

Hi, thanks for the PR!

Our CI should be running against your PR now so you can also check that to see your progress.

I've left a bunch of review comments on conventions, code style etc.

Ideally this PR doesn't change any behavior and only converts every piece. For this we'll also need to check the generated type definition file and compare it against the manually written one.

LekoArts · 2023-01-30T09:24:41Z

packages/gatsby-link/.babelrc

@@ -1 +1,8 @@
-{}
+{


This file change is not necessary since we use microbundle to generate the output.

You can check out https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-script to see how it's used (also has relevant package.json bits)

LekoArts · 2023-01-30T09:27:25Z

packages/gatsby-link/src/__tests__/index.tsx

 beforeEach(() => {
  global.__BASE_PATH__ = ``
  global.__PATH_PREFIX__ = ``
 })

 afterEach(cleanup)

+// eslint-disable-next-line @typescript-eslint/explicit-function-return-type


This PR has a lot of eslint-disable for the explicit function return type. We have that linting rule for a reason 😅

Please remove all instances of that eslint-disable and add the return types

LekoArts · 2023-01-30T09:27:53Z

packages/gatsby-link/src/__tests__/index.tsx

  global.__BASE_PATH__ = pathPrefix
  const source = createMemorySource(sourcePath)
  const history = createHistory(source)

  const utils = render(
    <LocationProvider history={history}>
      <Link
+        {...linkProps}


Why was that change necessary?

LekoArts · 2023-01-30T09:29:06Z

packages/gatsby-link/src/__tests__/index.tsx

@@ -386,7 +419,7 @@ describe(`ref forwarding`, () => {
  })

  it(`handles a RefObject (React >=16.4)`, () => {
-    const ref = React.createRef(null)
+    const ref = React.createRef()


Why was that change necessary?

LekoArts · 2023-01-30T09:30:27Z

packages/gatsby-link/index.d.ts

@@ -1,6 +1,17 @@
 import * as React from "react"
 import { NavigateFn, LinkProps } from "@reach/router" // These come from `@types/reach__router`

+declare global {


The types should be autogenerated from the source now and the manual file should no longer exist

LekoArts · 2023-01-30T09:35:41Z

packages/gatsby-link/src/__tests__/rewrite-link-path.ts


 beforeEach(() => {
  global.__TRAILING_SLASH__ = ``
  global.__PATH_PREFIX__ = undefined
 })

-const getRewriteLinkPath = (option = `legacy`, pathPrefix = undefined) => {
+const getRewriteLinkPath = (
+  option: string = `legacy`,


Instead of a string this should be the TrailingSlash type

LekoArts · 2023-01-30T09:41:46Z

packages/gatsby-link/src/index.tsx

@@ -110,7 +133,7 @@ class GatsbyLink extends React.Component {
      // If IO supported and element reference found, setup Observer functionality
      this.io = createIntersectionObserver(ref, inViewPort => {
        if (inViewPort) {
-          this.abortPrefetch = this._prefetch()
+          this.abortPrefetch = this._prefetch() as { abort: () => void } | null


Type-casting is a code smell, it seems like abortPrefetch is not properly typed then

LekoArts · 2023-01-30T09:43:20Z

packages/gatsby-link/src/index.tsx

-    this.abortPrefetch = null
-    this.handleRef = this.handleRef.bind(this)
+class GatsbyLink extends React.Component<IGatsbyLinkProps, IGatsbyLinkState> {
+  readonly state: IGatsbyLinkState = {


Two things here:

Removing the constructor doesn't seem necessary, you can also type that one: https://fettblog.eu/typescript-react/components/#class-components

According to https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/class_components/ the readonly isn't necessary

So I'd go with the constructor for now to keep the functional changes minimal in this PR (more straight-forward conversion)

LekoArts · 2023-01-30T09:45:37Z

packages/gatsby-link/src/rewrite-link-path.ts

@@ -1,23 +1,28 @@
 import { resolve } from "@gatsbyjs/reach-router"
 // Specific import to treeshake Node.js stuff
-import { applyTrailingSlashOption } from "gatsby-page-utils/apply-trailing-slash-option"
+import { applyTrailingSlashOption, type TrailingSlash } from "gatsby-page-utils"


The import from gatsby-page-utils/apply-trailing-slash-option was on purpose for better tree-shaking. So if you need the type from the root, add an additional import instead:

import type { TrailingSlash } from "gatsby-page-utils"

LekoArts · 2023-01-30T09:46:43Z

packages/gatsby-link/tsconfig.json

@@ -0,0 +1,3 @@
+{


Re-use what gatsby-script has:

{ "extends": "../../tsconfig.json", "exclude": ["node_modules", "src/__tests__", "dist"], "compilerOptions": { "declaration": false } }

Kornil · 2023-02-16T09:23:11Z

Sorry I got busy irl and kinda forgot about this PR, I'll address all the feedback these days!

Kornil added 4 commits January 28, 2023 01:51

Refactor config

01c11d7

Add new globals

bfac20b

Convert files to ts

fdf6c4c

Convert tests to ts

8e80c76

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 28, 2023

Kornil changed the title ~~Conver gatsby link to typescript~~ chore(gatsby): convert gatsby-link to typescript Jan 28, 2023

Merge branch 'master' into conver-gatsby-link-to-typescript

a40cc23

LekoArts added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 30, 2023

LekoArts suggested changes Jan 30, 2023

View reviewed changes

LekoArts added the topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) label Jan 30, 2023

tordans mentioned this pull request Feb 16, 2023

[TS] Link is incompatible with React.forwardRef due to ref type mismatch #34325

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gatsby): convert gatsby-link to typescript #37543

chore(gatsby): convert gatsby-link to typescript #37543

Kornil commented Jan 28, 2023

Kornil commented Jan 28, 2023 •

edited

Loading

LekoArts left a comment

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023

LekoArts Jan 30, 2023 •

edited

Loading

Kornil commented Feb 16, 2023

chore(gatsby): convert gatsby-link to typescript #37543

Are you sure you want to change the base?

chore(gatsby): convert gatsby-link to typescript #37543

Conversation

Kornil commented Jan 28, 2023

Description

Documentation

Related Issues

Kornil commented Jan 28, 2023 • edited Loading

LekoArts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LekoArts Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Kornil commented Feb 16, 2023

Kornil commented Jan 28, 2023 •

edited

Loading

LekoArts Jan 30, 2023 •

edited

Loading