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.

Sign up for GitHub

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

Jump to bottom

chore: Properly typecheck gatsby #31519

Merged
merged 12 commits into from
May 28, 2021
Merged

chore: Properly typecheck gatsby #31519

merged 12 commits into from
May 28, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented May 21, 2021 •
edited
Loading

Description

We had the wrong tsconfig.json 🙈 Only the exports/files included in src/internal.ts were typechecked

[ch31613]

@LekoArts LekoArts added the topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript label May 21, 2021
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 21, 2021
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 21, 2021
LekoArts
LekoArts commented May 25, 2021
View reviewed changes
packages/gatsby/src/cache/cache-fs.ts
@@ -54,7 +54,7 @@ exports.create = function (args): typeof DiskStore {
return new DiskStore(args && args.options ? args.options : args)
}

function DiskStore(options): void {
function DiskStore(this: any, options): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes TS2683: 'this' implicitly has type 'any' because it does not have a type annotation. throughout the file

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: about typing this as first param :O

packages/gatsby/src/commands/serve.ts Show resolved Hide resolved
packages/gatsby/src/internal-plugins/functions/gatsby-node.ts
@@ -168,7 +153,7 @@ const createWebpackConfig = async ({
// Logic is shared with webpack.config.js

// node env should be DEVELOPMENT | PRODUCTION as these are commonly used in node land
const nodeEnv = process.env.NODE_ENV || `${defaultNodeEnv}`
const nodeEnv = process.env.NODE_ENV || `development`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultNodeEnv wasn't set

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I copy/pasted that from webpack.config.js and that didn't get changed

packages/gatsby/src/bootstrap/schema-hot-reloader.ts Show resolved Hide resolved
packages/gatsby/src/commands/build.ts
@@ -196,7 +198,7 @@ module.exports = async function build(program: IBuildArgs): Promise<void> {
{ parentSpan: buildSpan }
)
buildSSRBundleActivityProgress.start()
let pageRenderer: string
let pageRenderer = ``
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Used before initialized error" - setting a default here will also assign it string

packages/gatsby/src/internal-plugins/functions/gatsby-node.ts Outdated Show resolved Hide resolved
packages/gatsby/src/utils/cache.ts
@@ -5,7 +5,7 @@ import manager, {
MultiCache,
} from "cache-manager"
import fs from "fs-extra"
import fsStore from "../cache/cache-fs"
import * as fsStore from "../cache/cache-fs"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Complained about "default export not existing"

@LekoArts LekoArts marked this pull request as ready for review May 26, 2021 08:15
KyleAMathews
KyleAMathews approved these changes May 27, 2021
View reviewed changes
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

👍 to functions changes! Thanks for finding this & all the cleanups 🚀

vladar
vladar approved these changes May 28, 2021
View reviewed changes
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

LGTM

wardpeet
wardpeet approved these changes May 28, 2021
View reviewed changes
packages/gatsby/src/cache/cache-fs.ts
@@ -54,7 +54,7 @@ exports.create = function (args): typeof DiskStore {
return new DiskStore(args && args.options ? args.options : args)
}

function DiskStore(options): void {
function DiskStore(this: any, options): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this equal to DiskStore? (I'm also new to this in typescript 😂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this was about having it implicitly:

TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.

I didn't want to type everything so this is the best middleground for now

packages/gatsby/src/cache/wrap-callback.ts
// eslint-disable-next-line @babel/no-invalid-this
const promise = fn.apply(this, args)
// @ts-ignore - unsure if fixing this introduces problems
const promise = fn.apply(this, args) // eslint-disable-line @babel/no-invalid-this
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't eslint-disable-next-line still work because next line is also a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it didn't work for me so I had to move it :/

packages/gatsby/src/utils/worker/render-html.ts
@@ -45,7 +41,7 @@ function clearCaches(): void {
const getStaticQueryPath = (hash: string): string =>
join(`page-data`, `sq`, `d`, `${hash}.json`)

const getStaticQueryResult = async (hash: string): any => {
const getStaticQueryResult = async (hash: string): Promise<any> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really any but we didn't type it before either so 👍

@LekoArts LekoArts merged commit 640ce36 into master May 28, 2021
@LekoArts LekoArts deleted the proper-ts-checking branch May 28, 2021 09:10
moonmeister added a commit to moonmeister/gatsby that referenced this pull request Jun 1, 2021
@moonmeister
Merge branch 'master' into feat/server-cache-headers
d805f3f 
* master: (23 commits)
  fix(gatsby-source-wordpress): Prevent "EADDRINUSE: address already in use 127.0.0.1" error (gatsbyjs#31713)
  feat(gatsby-source-wordpress: architecture.md (gatsbyjs#31537)
  chore(deps): update dependency @babel/node to ^7.14.2 (gatsbyjs#31690)
  chore(deps): update babel monorepo (gatsbyjs#31143)
  fix(gatsby): merge resolveType when merging abstract types (gatsbyjs#31710)
  chore(release): Publish next
  fix(gatsby): Correct config for svgo plugins whitelist
  chore: Add translations and validations to Contentful test (gatsbyjs#31533)
  chore(docs): Correct nginx spelling (gatsbyjs#31651)
  chore(docs): Update Jest instructions for v27 (gatsbyjs#31649)
  Fix typo in adding-search.md (gatsbyjs#31639)
  Fix typos in improving-build-performance.md (gatsbyjs#31640)
  feat(gatsby-source-wordpress): Fix false positive error if the URL and the responsePath are the same (gatsbyjs#31612)
  Fixed syntax error in example (gatsbyjs#31636)
  fix(contentful): pass reporter to retry function (gatsbyjs#31608)
  chore: Properly typecheck `gatsby` (gatsbyjs#31519)
  fix(gatsby-source-contentful): fix progress bar (gatsbyjs#31467)
  fix(gatsby-plugin-gatsby-cloud): fix cloud being bundled (gatsbyjs#31604)
  chore(gatsby-source-wordpress): Fix typos (gatsbyjs#31600)
  chore(docs): Add title to release notes (gatsbyjs#31595)
  ...
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
@gatsbybot @KyleAMathews
chore: Properly typecheck gatsby (gatsbyjs#31519)
7a1fc71 
Co-authored-by: Kyle Mathews <mathews.kyle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieh pieh pieh left review comments

@wardpeet wardpeet wardpeet approved these changes

@vladar vladar vladar approved these changes

@KyleAMathews KyleAMathews KyleAMathews approved these changes

Assignees
No one assigned
Labels
topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LekoArts @KyleAMathews @vladar @pieh @wardpeet