chore(gatsby): Convert jobs-manager to typescript #22189

martijnjanssen · 2020-03-11T21:30:46Z

Description

This converts the gatsby/util/jobs-manager and the corresponding tests to typescript, one more step towards a fully typed core codebase! I do think that I have gotten the lion's share of the types correct and don't think I have forgotten anything, but if I did, please say so 🙏

Related Issues

Related to #21995

packages/gatsby/src/utils/jobs-manager.ts

martijnjanssen · 2020-04-04T10:48:24Z

I completely forgot to push my changes. I have reverted the changes to the tests, since I cannot figure out how to handle module isolation in typescript.

pvdz · 2020-04-09T08:13:34Z

(I've converted this PR into a Draft PR)

martijnjanssen · 2020-04-09T08:37:58Z

This is not a draft, the PR is finished in my opinion, just waiting for a final review. I should've removed that label 🙂

pvdz

Hey the PR looks good to me. One change I'd like to request is to move the types to the top directly after the imports. Other comments are minor or can be ignored for this PR. Thank you!

packages/gatsby/src/utils/jobs-manager.ts

gatsby-cloud-staging · 2020-07-17T14:15:33Z

Gatsby Cloud Build Report

🚩 Your build failed. See the build logs here

Errors

Error in "/usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/gatsby-node.js": Cannot find module 'gatsby-cli/lib/reporter'
Require stack:
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/is-valid-collection-path-implementation.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/create-pages-from-collection-builder.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/create-page-wrapper.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby-plugin-page-creator/gatsby-node.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/resolve-module-exports.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/validate.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/load.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/load-plugins/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/services/initialize.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/services/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bootstrap/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/commands/build.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/node_modules/gatsby-cli/lib/create-cli.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/node_modules/gatsby-cli/lib/index.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/dist/bin/gatsby.js
- /usr/src/app/www/examples/gatsbygram/node_modules/gatsby/cli.js

pvdz · 2020-07-17T14:19:29Z

Ah looks like WorkerError has to go above the types (because classes aren't hoisted). Sorry about that.

martijnjanssen · 2020-07-17T14:48:53Z

Looks like my editor removed all trailing ,'s... I'm adding them back now.

martijnjanssen · 2020-07-17T14:56:58Z

Okay there are some pre-commit tasks which keep formatting the file and removing trailing ,'s from the file, would you know anything about that @pvdz?

pvdz · 2020-07-17T17:03:55Z

I believe it's caused by formatting? What happens when you do yarn format? (or npm run format)

I'll proc the formatting from a bot (this will cause remote changes for you. When necessary, either pull before pushing or force push)

pvdz · 2020-07-17T17:05:18Z

I'm actually about to go on vacation so if we can't resolve that before then, somebody else will take over. But I think this is about ready to merge.

pvdz · 2020-07-17T17:12:36Z

This seems like an actual problem:

../gatsby/src/redux/types.ts(6,10): error TS2305: Module '"../utils/jobs-manager"' has no exported member 'InternalJobInterface'.

Looks like you can repro this locally by running yarn bootstrap, which pretty much compiles all the packages. And probably for one package specifically although the output is not clear on that. Good bet that it's just the gatsby package itself :)

pvdz · 2020-07-29T13:13:54Z

@martijnjanssen friendly ping :)

pvdz · 2020-08-11T11:06:12Z

I've went ahead and fixed the two remaining typing problems.

blainekasten

This looks great to me, let's merge it!

Thank you so much for contributing to our TypeScript refactor! We have more work to do and we would love to have you stay involved in our transition. Please submit more PRs! 💜

you fixed it!

* chore(gatsby) convert jobs-manager to typescript * chore(gatsby) revert jobs-manager tests to js * chore(gatsby) fix formatting for jobs-manager * chore(gatsby) move imports for jobs-manager after imports * Chore(gatsby) check contentDigest of job when checking internalJob * Chore(gatsby) declare WorkerError before usage * chore: format * Fix types Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com> Co-authored-by: Peter van der Zee <github-public@qfox.nl>

martijnjanssen requested a review from a team as a code owner March 11, 2020 21:30

martijnjanssen force-pushed the jobs-manager-typescript-convert branch from 9b4bfd3 to 1d9d07e Compare March 11, 2020 21:37

martijnjanssen added the type: TypeScript migration label Mar 11, 2020

martijnjanssen force-pushed the jobs-manager-typescript-convert branch from 1d9d07e to aebcb60 Compare March 11, 2020 21:53

martijnjanssen added the status: WIP label Mar 11, 2020

blainekasten reviewed Mar 13, 2020

View reviewed changes

packages/gatsby/src/utils/jobs-manager.ts Outdated Show resolved Hide resolved

blainekasten reviewed Mar 13, 2020

View reviewed changes

packages/gatsby/src/utils/jobs-manager.ts Outdated Show resolved Hide resolved

martijnjanssen force-pushed the jobs-manager-typescript-convert branch from aebcb60 to 811fd6f Compare April 4, 2020 10:45

martijnjanssen requested a review from blainekasten April 4, 2020 14:03

pvdz marked this pull request as draft April 9, 2020 08:13

pvdz removed the status: WIP label Apr 9, 2020

martijnjanssen marked this pull request as ready for review April 9, 2020 08:42

pvdz previously requested changes Jul 17, 2020

View reviewed changes

packages/gatsby/src/utils/jobs-manager.ts Outdated Show resolved Hide resolved

packages/gatsby/src/utils/jobs-manager.ts Show resolved Hide resolved

packages/gatsby/src/utils/jobs-manager.ts Show resolved Hide resolved

packages/gatsby/src/utils/jobs-manager.ts Outdated Show resolved Hide resolved

martijnjanssen added 4 commits July 17, 2020 16:09

chore(gatsby) convert jobs-manager to typescript

934f09c

chore(gatsby) revert jobs-manager tests to js

1a2f7da

chore(gatsby) fix formatting for jobs-manager

5172061

chore(gatsby) move imports for jobs-manager after imports

66bf919

martijnjanssen force-pushed the jobs-manager-typescript-convert branch from 811fd6f to 66bf919 Compare July 17, 2020 14:12

martijnjanssen added 2 commits July 17, 2020 16:26

Chore(gatsby) check contentDigest of job when checking internalJob

13be45b

Chore(gatsby) declare WorkerError before usage

1d1745f

chore: format

3707e36

blainekasten mentioned this pull request Jul 17, 2020

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

Closed

Fix types

6853038

pvdz removed the request for review from blainekasten August 11, 2020 11:07

pvdz changed the title ~~Convert jobs-manager to typescript~~ chore(gatsby): Convert jobs-manager to typescript Aug 11, 2020

Merge branch 'master' into jobs-manager-typescript-convert

526a745

gatsbyjs deleted a comment from gatsby-cloud bot Aug 11, 2020

blainekasten approved these changes Aug 11, 2020

View reviewed changes

blainekasten added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Aug 11, 2020

blainekasten merged commit f59fb37 into master Aug 11, 2020

blainekasten deleted the jobs-manager-typescript-convert branch August 11, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gatsby): Convert jobs-manager to typescript #22189

chore(gatsby): Convert jobs-manager to typescript #22189

martijnjanssen commented Mar 11, 2020

martijnjanssen commented Apr 4, 2020

pvdz commented Apr 9, 2020

martijnjanssen commented Apr 9, 2020

pvdz left a comment

gatsby-cloud-staging bot commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 17, 2020 •

edited

Loading

martijnjanssen commented Jul 17, 2020

martijnjanssen commented Jul 17, 2020

pvdz commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 17, 2020

pvdz commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 29, 2020

pvdz commented Aug 11, 2020

blainekasten left a comment

chore(gatsby): Convert jobs-manager to typescript #22189

chore(gatsby): Convert jobs-manager to typescript #22189

Conversation

martijnjanssen commented Mar 11, 2020

Description

Related Issues

martijnjanssen commented Apr 4, 2020

pvdz commented Apr 9, 2020

martijnjanssen commented Apr 9, 2020

pvdz left a comment

Choose a reason for hiding this comment

gatsby-cloud-staging bot commented Jul 17, 2020 • edited Loading

Gatsby Cloud Build Report

pvdz commented Jul 17, 2020 • edited Loading

martijnjanssen commented Jul 17, 2020

martijnjanssen commented Jul 17, 2020

pvdz commented Jul 17, 2020 • edited Loading

pvdz commented Jul 17, 2020

pvdz commented Jul 17, 2020 • edited Loading

pvdz commented Jul 29, 2020

pvdz commented Aug 11, 2020

blainekasten left a comment

Choose a reason for hiding this comment

gatsby-cloud-staging bot commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 17, 2020 •

edited

Loading

pvdz commented Jul 17, 2020 •

edited

Loading