chore: Migrate custom store and queue to TypeScript #25389

chooban · 2020-06-29T17:15:26Z

Description

This migrates three files to TypeScript:

gatsby/src/query/queue.js
gatsby/src/query/better-queue-custom-store.js (and its test)

Related Issues

Related to #21995

This migrates the custom store and the queue to typescript.

chooban · 2020-06-29T17:17:24Z

packages/gatsby/src/query/better-queue-custom-store.ts

-    takeFirstN: function (n, cb) {
-      const lockId = uuid++
+    takeFirstN: function (n, cb): void {
+      const lockId = `` + uuid++


⚠️ So this has changed. The interface from better-queue states that the lockId should be a string.

Nit: I think I would prefer String(uuid++) for clarity.

chooban · 2020-06-29T17:18:13Z

packages/gatsby/src/query/queue.ts

+import { ProgressActivityTracker } from "../.."
+
+export type Task = any
+type TaskResult = any


I couldn't locate a better type, but at least we can just change these lines if one exists.

chooban · 2020-06-29T17:18:52Z

packages/gatsby/src/query/queue.ts

  if (jobs.length === 0) {
    return Promise.resolve()
  }

  return new Promise((resolve, reject) => {
    let taskFinishCallback
+
+    const gc = (): void => {
+      // eslint-disable-next-line @typescript-eslint/no-use-before-define


gc depends on taskFailedCallback and taskFailedCallback depends on gc.

packages/gatsby/src/query/better-queue-custom-store.ts

pvdz

Next time, could you please use Array<> rather than [] for array typing? This isn't formalized yet but we are likely to move into that direction soon (consistency with Map, Set, Promise, etc).

I'm a little surprised TS is not asking you to type the cb functions, but if it doesn't I guess that's okay.

Most of my comments are nits. Can you at least apply the String(uuid++) cases? And maybe look at the others. This looks good otherwise, thanks! :)

packages/gatsby/src/query/better-queue-custom-store.ts

pvdz · 2020-06-30T11:58:26Z

packages/gatsby/src/query/better-queue-custom-store.ts

          oldPriorityTasks.splice(oldPriorityTasks.indexOf(taskId), 1)

          if (
            addTaskWithPriority(taskId, priority) ||
-            oldPriority.length === 0
+            oldPriorityTasks.length === 0


Was this a bug?

Yes, I believe so. The oldPriority value previously used was a number (or null) so it has no length field and TS complained. It seems logical to me that the intention was to check the lenght of the old tasks instead. Annoyingly, no test failures.

I think what it's saying is if calling code attempts to add the same task twice at different priorities, and removing the older task results in an empty set of tasks for the original priority, then the priority keys need updating. I can try and write a test to cover that.

pvdz · 2020-06-30T11:59:34Z

packages/gatsby/src/query/better-queue-custom-store.ts

-    takeFirstN: function (n, cb) {
-      const lockId = uuid++
+    takeFirstN: function (n, cb): void {
+      const lockId = `` + uuid++


Nit: I think I would prefer String(uuid++) for clarity.

packages/gatsby/src/query/better-queue-custom-store.ts

chooban · 2020-06-30T13:28:33Z

This is an update of two halves (well, commits). The first deals with the nitpicks and style updates, the second those untyped callbacks.

By changing the invocation of Queue to use a full Options object, we can get the types from that and the callbacks all check out. I was a little wary, especially given it's not explicitly tested, but I checked the source for better-queue and I think it's fine.

We were initially passing a handler value as the first argument, and that's then assigned to the process field on options. Instead, we put it as a the process option ourself and pass just that config object.

Rather than leaking the extra function to the outside world, we'll keep it internal. The tests do not cause an issue as they're written in a way that erases types.

chooban · 2020-07-01T07:29:46Z

Having slept on it, I pushed up a very small change. It avoids leaking the additional method that's been added to the Store interface further into the codebase.

At the moment, the tests are not typesafe as the Store object is run through pify first. It should be straightforward enough to refactor that as well, possibly removing pify, if required.

chooban · 2020-07-13T11:13:29Z

Merged up to date with master, and included the recent changes to the queue file.

pvdz

Awesome! Do you want to update the last comment I made or leave it? :)

pvdz · 2020-07-13T12:42:00Z

packages/gatsby/src/query/__tests__/better-queue-custom-store.ts

+    let lockId
+    let tasks


Nit: you're folding up the declaration with the initialization above (correctly, I think), why not here?

Just missed it, I'm afraid. Pushing up a commit for it now.

pvdz · 2020-07-13T14:04:37Z

Awesome! :D Thanks 💜

* WIP * chore: Migrate custom store to TypeScript This migrates the custom store and the queue to typescript. * Test types * Remove warning * chore: Pass full config object * Do not leak interface extension details Rather than leaking the extra function to the outside world, we'll keep it internal. The tests do not cause an issue as they're written in a way that erases types. * Bring in queue changes * Initialize and declare initial values

chooban added 3 commits June 29, 2020 17:21

WIP

3d0d3e4

chore: Migrate custom store to TypeScript

05809dc

This migrates the custom store and the queue to typescript.

Test types

2d53ce8

chooban added the topic: TypeScript migration label Jun 29, 2020

chooban requested a review from a team as a code owner June 29, 2020 17:15

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 29, 2020

chooban changed the title ~~Ts migrate/better queue store~~ chore: Migrate custom store and queue to TypeScript Jun 29, 2020

chooban commented Jun 29, 2020

View reviewed changes

blainekasten mentioned this pull request Jun 29, 2020

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

Closed

chooban commented Jun 29, 2020

View reviewed changes

packages/gatsby/src/query/better-queue-custom-store.ts Outdated Show resolved Hide resolved

Remove warning

a943c62

pvdz added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 30, 2020

pvdz suggested changes Jun 30, 2020

View reviewed changes

chore: Pass full config object

94025b2

Do not leak interface extension details

4551a52

Rather than leaking the extra function to the outside world, we'll keep it internal. The tests do not cause an issue as they're written in a way that erases types.

chooban added 5 commits July 2, 2020 14:16

Merge branch 'master' into ts-migrate/better-queue-store

2e6baf0

Merge branch 'master' into ts-migrate/better-queue-store

ef7dfc8

Merge branch 'master' into ts-migrate/better-queue-store

6158818

Bring in queue changes

56bd7e0

Merge branch 'master' into ts-migrate/better-queue-store

07c4f17

chooban requested a review from pvdz July 13, 2020 11:13

pvdz previously approved these changes Jul 13, 2020

View reviewed changes

Initialize and declare initial values

7b8564d

chooban dismissed pvdz’s stale review via 7b8564d July 13, 2020 13:41

pvdz approved these changes Jul 13, 2020

View reviewed changes

pvdz merged commit 79348f9 into gatsbyjs:master Jul 13, 2020

chooban deleted the ts-migrate/better-queue-store branch July 13, 2020 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrate custom store and queue to TypeScript #25389

chore: Migrate custom store and queue to TypeScript #25389

chooban commented Jun 29, 2020

chooban Jun 29, 2020

pvdz Jun 30, 2020

chooban Jun 29, 2020

chooban Jun 29, 2020

pvdz left a comment

pvdz Jun 30, 2020

chooban Jun 30, 2020

pvdz Jun 30, 2020

chooban commented Jun 30, 2020

chooban commented Jul 1, 2020

chooban commented Jul 13, 2020

pvdz left a comment

pvdz Jul 13, 2020

chooban Jul 13, 2020

pvdz commented Jul 13, 2020

chore: Migrate custom store and queue to TypeScript #25389

chore: Migrate custom store and queue to TypeScript #25389

Conversation

chooban commented Jun 29, 2020

Description

Related Issues

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

pvdz 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

chooban commented Jun 30, 2020

chooban commented Jul 1, 2020

chooban commented Jul 13, 2020

pvdz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdz commented Jul 13, 2020