Skip to content
This repository was archived by the owner on Feb 4, 2025. It is now read-only.

Add types for jobs - #16

Merged
mariusandra merged 5 commits into
mainfrom
jobs
May 7, 2021
Merged

Add types for jobs#16
mariusandra merged 5 commits into
mainfrom
jobs

Conversation

@mariusandra

Copy link
Copy Markdown
Collaborator

This is a WIP that adds types to an experimental syntax for jobs as described here: PostHog/plugin-server#325

The following will work after this is merged:

import { CreatePluginMeta, MetaJobsInput } from '@posthog/plugin-scaffold'

type Meta = CreatePluginMeta<{
    config: {
        localhostIP: string
    }
    attachments: {
        maxmindMmdb?: PluginAttachment
    }
    global: {
        ipLookup?: Reader<Response>
    }
    jobs: {
        flushBatch: (ops: { batch: string[]; retryCount?: number }) => Promise<void>
    }
}>

export const jobs: MetaJobsInput<Meta> = {
    flushBatch: async ({ batch, retryCount = 0 }, { jobs }) => {
        const resp = await fetch('https://httpbin.org/post', {
            method: 'post',
            body: JSON.stringify(batch),
            headers: { 'Content-Type': 'application/json' },
        })
        if (resp.status !== 200) {
            if (retryCount > 5) {
                console.error('Could not post batch', batch)
                return
            }
            jobs.runIn(30, 'seconds').flushBatch({ batch, retryCount: retryCount + 1 })
        }
    },
}

export async function setupPlugin({ jobs }: Meta) {
    await jobs.runNow().flushBatch({ batch: [] })
}

The meta.jobs is fully typed. The jobs export unfortunately has to go through MetaJobsInput<Meta> when declaring (or it could be Meta['__jobsInput'] perhaps?), as the output of meta.jobs has a different structure than the input. meta.jobs runs all the jobs via a wrapper like jobs.runIn(30, 'minutes').flushBatch()....

Obviously this should be merged only once we agree on the structure...

@yakkomajuri yakkomajuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not particularly opinionated, but my one comment would be that I would intuitively expect to pass a function as a parameter to another to run a task/job. That's the case with setTimeout, setInterval, node-schedule, etc.

Where instead of:

jobs.runNow().flushBatch({ batch: [] })

We'd have:

jobs.runNow(() => flushBatch({ batch: [] }))

// or

jobs.runNow(someJobFunc)

This discussion is completely external to the implementation, so just speaking about what would feel "natural" to me. I do understand we have a tradeoff between feel and the implementation behind the scenes.

And like I said, not very opinionated.

Comment thread src/types.ts Outdated
Comment thread src/types.ts Outdated
@mariusandra

Copy link
Copy Markdown
Collaborator Author

It might feel natural, but like you said yourself, the implementation gets in the way. We have no idea when this function will be run... nor on which server. The only thing we know is that we need to store jobName and payload in the database somehow.

Within those constraints I'm open to any kind of suggestion or simplification of the API!

@Twixes

Twixes commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

What file is the implementation of this in (in PostHog/plugin-server#325)?

@Twixes

Twixes commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

Ah, so it's PostHog/plugin-server#351

@Twixes Twixes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this needs reworking because of jobs API changes PostHog/plugin-server#351 (comment)

@Twixes Twixes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, wrong review, I wanted this to be 🔴 🟥 🙅 RED

@mariusandra
mariusandra requested a review from Twixes May 6, 2021 09:10
@mariusandra

Copy link
Copy Markdown
Collaborator Author

Let's try again!

@mariusandra

Copy link
Copy Markdown
Collaborator Author

Here's a plugin using this code: https://github.com/PostHog/session-tracker-plugin/pull/2/files

@Twixes Twixes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Made two suggestions, implemented them in #17

Comment thread src/types.ts Outdated
: Record<string, (opts: any) => JobControls>
}

export type MetaJobsInput<M extends PluginMeta> = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this could be called PluginJobs, which would be more similar to other such types and more obvious?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good for me

Comment thread src/types.ts Outdated
[K in keyof J]: (opts: J[K]) => JobControls
}

export type CreatePluginMeta<Input extends MetaInput> = PluginMeta & {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is named like a function (starting with a verb) on top of plain PluginMeta, but if this is the way we'd like devs to use for typing, we should only expose this really. Could we rename this to PluginMeta and rename the "dumb" base interface to BasePluginMeta?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great!

@mariusandra
mariusandra merged commit 975c392 into main May 7, 2021
@mariusandra
mariusandra deleted the jobs branch May 7, 2021 11:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants