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

chore: Migrate custom store and queue to TypeScript #25389

Merged
merged 12 commits into from
Jul 13, 2020
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"@babel/runtime": "^7.10.3",
"@lerna/prompt": "3.18.5",
"@types/babel__code-frame": "^7.0.1",
"@types/better-queue": "^3.8.2",
"@types/bluebird": "^3.5.30",
"@types/cache-manager": "^2.10.2",
"@types/common-tags": "^1.8.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const MemoryStoreWithPriorityBuckets = require(`../better-queue-custom-store`)
const pify = require(`pify`)
import { MemoryStoreWithPriorityBuckets } from "../better-queue-custom-store"
import pify from "pify"

// those are tests copied from https://github.com/diamondio/better-queue-store-test/blob/master/tester.js
// and converted from mocha to jest + used pify to make it nicer to read than callback chain
Expand All @@ -18,6 +18,7 @@ describe(`Custom better-queue memory store`, () => {
`releaseLock`,
]
beforeEach(() => {
// eslint-disable-next-line new-cap
store = MemoryStoreWithPriorityBuckets()
functions.forEach(fnName => {
if (store[fnName]) {
Expand Down Expand Up @@ -51,10 +52,8 @@ describe(`Custom better-queue memory store`, () => {
await store.putTask(`task2`, { value: `secret 2` }, 1)
await store.putTask(`task3`, { value: `secret 3` }, 1)

let lockId, tasks

lockId = await store.takeLastN(2)
tasks = await store.getLock(lockId)
let lockId: string = await store.takeLastN(2)
let tasks: any = await store.getLock(lockId)

// should get the third task
expect(tasks.task3.value).toBe(`secret 3`)
Expand All @@ -80,7 +79,8 @@ describe(`Custom better-queue memory store`, () => {
await store.putTask(`task2`, { value: `secret 2` }, 1)
await store.putTask(`task3`, { value: `secret 3` }, 1)

let lockId, tasks
let lockId
let tasks
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


lockId = await store.takeFirstN(2)
tasks = await store.getLock(lockId)
Expand Down Expand Up @@ -109,8 +109,8 @@ describe(`Custom better-queue memory store`, () => {
await store.putTask(`task2`, { value: `secret 2` }, 1)
await store.putTask(`task3`, { value: `secret 3` }, 1)

const lock1 = await store.takeFirstN(1)
const lock2 = await store.takeLastN(1)
const lock1: string = await store.takeFirstN(1)
const lock2: string = await store.takeLastN(1)

let workers

Expand Down Expand Up @@ -151,7 +151,7 @@ describe(`Custom better-queue memory store`, () => {
await store.deleteTask(`task2`)

// take 2
const lockId = await store.takeFirstN(2)
const lockId: string = await store.takeFirstN(2)
const tasks = await store.getLock(lockId)

// should get the first task
Expand All @@ -173,9 +173,8 @@ describe(`Custom better-queue memory store`, () => {
await store.putTask(`task4`, { value: `secret 4` }, 2)

// take first 2
let lockId, tasks
lockId = await store.takeFirstN(2)
tasks = await store.getLock(lockId)
let lockId: string = await store.takeFirstN(2)
let tasks = await store.getLock(lockId)

// should get the third task
expect(tasks.task3.value).toBe(`secret 3`)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,32 +1,40 @@
function MemoryStoreWithPriorityBuckets() {
import { Store } from "better-queue"

// getRunningTasks is an extension to the interface, and is used in the tests
interface IGatsbyBetterStore<T> extends Store<T> {
pvdz marked this conversation as resolved.
Show resolved Hide resolved
getRunningTasks(cb: (error: any, runningTasks: any) => void): void
}

export function MemoryStoreWithPriorityBuckets<T>(): IGatsbyBetterStore<T> {
chooban marked this conversation as resolved.
Show resolved Hide resolved
type RunningTasks = Record<string, T>
let uuid = 0

/**
* Task ids grouped by priority
*/
const queueMap = new Map()
const queueMap = new Map<number, string[]>()

/**
* Task id to task lookup
*/
const tasks = new Map()
const tasks = new Map<string, T>()

/**
* Task id to priority lookup
*/
const taskIdToPriority = new Map()
const taskIdToPriority = new Map<string, number>()

/**
* Lock to running tasks object
*/
const running = {}
const running: Record<string, RunningTasks> = {}

let priorityKeys = []
const updatePriorityKeys = () => {
let priorityKeys: number[] = []
const updatePriorityKeys = (): void => {
priorityKeys = Array.from(queueMap.keys()).sort((a, b) => b - a)
}

const addTaskWithPriority = (taskId, priority) => {
const addTaskWithPriority = (taskId: string, priority: number): boolean => {
let needToUpdatePriorityKeys = false
let priorityTasks = queueMap.get(priority)
if (!priorityTasks) {
Expand All @@ -41,36 +49,39 @@ function MemoryStoreWithPriorityBuckets() {
}

return {
connect: function (cb) {
connect: function (cb): void {
cb(null, tasks.size)
},
getTask: function (taskId, cb) {
getTask: function (taskId, cb): void {
// @ts-ignore
cb(null, tasks.get(taskId))
},
deleteTask: function (taskId, cb) {
deleteTask: function (taskId, cb): void {
if (tasks.get(taskId)) {
tasks.delete(taskId)
const priority = taskIdToPriority.get(taskId)
const priorityTasks = queueMap.get(priority)
priorityTasks.splice(priorityTasks.indexOf(taskId), 1)
taskIdToPriority.delete(taskId)
if (priority) {
const priorityTasks = queueMap.get(priority) || []
chooban marked this conversation as resolved.
Show resolved Hide resolved
priorityTasks.splice(priorityTasks.indexOf(taskId), 1)
taskIdToPriority.delete(taskId)
}
}
cb()
},
putTask: function (taskId, task, priority = 0, cb) {
putTask: function (taskId, task, priority = 0, cb): void {
const oldTask = tasks.get(taskId)
tasks.set(taskId, task)
let needToUpdatePriorityKeys = false
if (oldTask) {
const oldPriority = taskIdToPriority.get(taskId)

if (oldPriority !== priority) {
const oldPriorityTasks = queueMap.get(oldPriority)
if (oldPriority && oldPriority !== priority) {
const oldPriorityTasks = queueMap.get(oldPriority) || []
chooban marked this conversation as resolved.
Show resolved Hide resolved
oldPriorityTasks.splice(oldPriorityTasks.indexOf(taskId), 1)

if (
addTaskWithPriority(taskId, priority) ||
oldPriority.length === 0
oldPriorityTasks.length === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

) {
needToUpdatePriorityKeys = true
}
Expand All @@ -82,29 +93,33 @@ function MemoryStoreWithPriorityBuckets() {
if (needToUpdatePriorityKeys) {
updatePriorityKeys()
}
cb()
cb(null)
},
takeFirstN: function (n, cb) {
const lockId = uuid++
takeFirstN: function (n, cb): void {
const lockId = `` + uuid++
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 has changed. The interface from better-queue states that the lockId should be a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

let remainingTasks = n
let needToUpdatePriorityKeys = false
let haveSomeTasks = false
const tasksToRun = {}
const tasksToRun: RunningTasks = {}

for (const priority of priorityKeys) {
const taskWithSamePriority = queueMap.get(priority)
const grabbedTaskIds = taskWithSamePriority.splice(0, remainingTasks)
const tasksWithSamePriority = queueMap.get(priority)
const grabbedTaskIds =
tasksWithSamePriority?.splice(0, remainingTasks) ?? []
grabbedTaskIds.forEach(taskId => {
// add task to task that will run
// and remove it from waiting list
tasksToRun[taskId] = tasks.get(taskId)
tasks.delete(taskId)
taskIdToPriority.delete(taskId)
haveSomeTasks = true
const task = tasks.get(taskId)
if (task) {
tasksToRun[taskId] = task
tasks.delete(taskId)
taskIdToPriority.delete(taskId)
haveSomeTasks = true
}
})

remainingTasks -= grabbedTaskIds.length
if (taskWithSamePriority.length === 0) {
if (tasksWithSamePriority?.length === 0) {
queueMap.delete(priority)
needToUpdatePriorityKeys = true
}
Expand All @@ -123,26 +138,26 @@ function MemoryStoreWithPriorityBuckets() {

cb(null, lockId)
},
takeLastN: function (n, cb) {
takeLastN: function (n, cb): void {
// This is not really used by Gatsby, but will be implemented for
// completion in easiest possible way (so not very performant).
// Mostly done so generic test suite used by other stores passes.
// This is mostly C&P from takeFirstN, with array reversal and different
// splice args
const lockId = uuid++
const lockId = `` + uuid++
chooban marked this conversation as resolved.
Show resolved Hide resolved
let remainingTasks = n
let needToUpdatePriorityKeys = false
let haveSomeTasks = false
const tasksToRun = {}

for (const priority of priorityKeys.reverse()) {
const taskWithSamePriority = queueMap.get(priority)
const tasksWithSamePriority = queueMap.get(priority) || []
chooban marked this conversation as resolved.
Show resolved Hide resolved
const deleteCount = Math.min(
remainingTasks,
taskWithSamePriority.length
tasksWithSamePriority.length
)
const grabbedTaskIds = taskWithSamePriority.splice(
taskWithSamePriority.length - deleteCount,
const grabbedTaskIds = tasksWithSamePriority.splice(
tasksWithSamePriority.length - deleteCount,
deleteCount
)
grabbedTaskIds.forEach(taskId => {
Expand All @@ -155,7 +170,7 @@ function MemoryStoreWithPriorityBuckets() {
})

remainingTasks -= grabbedTaskIds.length
if (taskWithSamePriority.length === 0) {
if (tasksWithSamePriority.length === 0) {
queueMap.delete(priority)
needToUpdatePriorityKeys = true
}
Expand All @@ -174,17 +189,15 @@ function MemoryStoreWithPriorityBuckets() {

cb(null, lockId)
},
getRunningTasks: function (cb) {
getRunningTasks: function (cb): void {
cb(null, running)
},
getLock: function (lockId, cb) {
getLock: function (lockId, cb): void {
cb(null, running[lockId])
},
releaseLock: function (lockId, cb) {
releaseLock: function (lockId, cb): void {
delete running[lockId]
cb()
cb(null)
},
}
}

module.exports = MemoryStoreWithPriorityBuckets
Loading