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

Allow specifying function with appId when invoking #449

Merged
merged 32 commits into from
Jan 12, 2024
Merged

Conversation

jpwilliams
Copy link
Member

@jpwilliams jpwilliams commented Jan 3, 2024

Summary

When using step.invoke(), the provided function option can be either an InngestFunction instance or a string.

In the latter case, the user is required to also include the ID of the app being called, which is unclear. This PR makes a few changes to accommodate this nicety.

Awkwardly, the functionality of function: string must stay the same to avoid a breaking change. This makes adding a new functionId option a bit confusing, as now I can specify both function: string and functionId: string.

A related issue for invocation is wanting to import only a function's types to avoid also importing all of another function's dependencies when invoking. This also applies to cross-app invocation, where a user may want to invoke a function using a string ID and simultaneously provide typing or schemas for validating input/output.

This change requires some form of referencing a function using only an ID and relying on either the types of an InngestFunction instance or some inputted types/schemas.

Broadly, we add a new referenceFunction export (I don't like the name of it) which can be passed in as the function being invoked. We should also deprecate use of function: string and prefer creating a referenceFunction.

Currently, and unchanged to avoid a breaking change

A "full" function ID, which is [clientId]-[functionId].
Input and output types are unknown.

await step.invoke("start-process", {
  function: "some-python-app-some-python-fn",
});

Currently, and unchanged

Input and output types are known.

import { someInngestFn } from "@/inngest/someFn";

await step.invoke("start-process", {
  function: someInngestFn,
});

Referencing a function using an existing function instance

No dependencies of the target function are imported.

The function ID must be provided at runtime, but we can enforce that this is the literal ID given to the function, i.e. functionId: "some-fn" instead of just functionId: string.

Input and output types are known.

import { referenceFunction } from "inngest";
import { type someInngestFn } from "@/inngest/someFn";

await step.invoke("start-process", {
  function: referenceFunction<typeof someInngestFn>({
    functionId: "some-fn",
  }),
});

Referencing some external function by ID

The app ID of the client executing the call is used, as we don't specify an appId.
Input and output types are unknown.

import { referenceFunction } from "inngest";

await step.invoke("start-process", {
  function: referenceFunction({
    functionId: "some-fn",
  }),
});

Referencing some external function by ID and app ID

We specify an app ID here to form the full ID instead of using the executing client's ID.
Input and output types are unknown.

import { referenceFunction } from "inngest";

await step.invoke("start-process", {
  function: referenceFunction({
    functionId: "some-fn",
    appId: "some-app",
  }),
});

Specifying types for the input and output of a reference function

We can also provide input and output schemas to add typing to our reference function, which adds types to the required input and output of step.invoke().

Providing a schema is optional and you can also choose to provide only an input or only an output.

Note

If you've passed an actual Inngest function like referenceFunction<typeof someFn>({ ... }), you cannot add additional schemas.

For the purposes of this example, we use Zod, but this will expand to allow many validation libraries (or types directly) in the future. This will complement being able to provide schemas on a function directly in the future, as well as validating I/O instead of only using types.

import { referenceFunction } from "inngest";
import { z } from "zod";

await step.invoke("start-process", {
  function: referenceFunction({
    functionId: "some-fn",
    appId: "some-app",
    schemas: {
      data: z.object({
        foo: z.string(),
      }),
      return: z.object({
        success: z.boolean(),
      }),
    },
  }),
});

Reusing reference functions

It's likely healthy for a user to declare reference functions the same way they do all of their others, meaning importing and invoking it is the same experience for both local and remote functions.

For example, if a local function is declared like so:

// src/inngest/localFn.ts
import { inngest } from "@/inngest";

export default inngest.createFunction(/* ... */);

Then a remote function may be declared like so:

// src/inngest/remoteFn.ts
import { referenceFunction } from "@/inngest";

export default referenceFunction(/* ... */);

In both instances, the function is then simply imported and invoked.

// src/inngest/someFn.ts
import { inngest } from "@/inngest";
import { localFn } from "@/inngest/localFn";
import { remoteFn } from "@/inngest/remoteFn";

export default inngest.createFunction(
  { id: "some-fn" },
  { event: "some-event" },
  async ({ step }) => {
    return Promise.all([
      step.invoke({ function: localFn }),
      step.invoke({ function: remoteFn }),
    ]);
  },
);

Checklist

  • Added a docs PR that references this PR
  • Added unit/integration tests
  • Added changesets if applicable
  • Add runtime warning for use of function: string when invoking; deprecate in v4
  • Rename input to data
  • Rename output to return
  • Ensure both function_id and app_id are sent during invocation
  • Discuss how function_id and app_id should be sent together

Related

@jpwilliams jpwilliams added the ⬆️ improvement Performance, reliability, or usability improvements label Jan 3, 2024
@jpwilliams jpwilliams self-assigned this Jan 3, 2024
Copy link

changeset-bot bot commented Jan 3, 2024

🦋 Changeset detected

Latest commit: 8ea53e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
inngest Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@inngest-release-bot inngest-release-bot added the 📦 inngest Affects the `inngest` package label Jan 3, 2024
@goodoldneon
Copy link
Contributor

Is this a breaking change for users already specifying function: appID + "-" + functionID?

Copy link
Contributor

@tonyhb tonyhb left a comment

Choose a reason for hiding this comment

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

LGTM - great work :)

@tonyhb
Copy link
Contributor

tonyhb commented Jan 3, 2024

Is this a breaking change for users already specifying function: appID + "-" + functionID?

https://github.com/inngest/inngest-js/pull/449/files#diff-498e58b5ce5bcba89da57c0c48881bb031a6526cf06b8af45e72deeb71644316R410-R412

Looks like this filters out empty strings before joining to make it backwards compatible

@goodoldneon
Copy link
Contributor

Is this a breaking change for users already specifying function: appID + "-" + functionID?

https://github.com/inngest/inngest-js/pull/449/files#diff-498e58b5ce5bcba89da57c0c48881bb031a6526cf06b8af45e72deeb71644316R410-R412

Looks like this filters out empty strings before joining to make it backwards compatible

But client.id will always be a string, right? Seems like setting function to the full-qualified ID (with app ID) will result in a string twice prefixed with the app ID

@jpwilliams jpwilliams marked this pull request as draft January 4, 2024 22:47
@jpwilliams jpwilliams marked this pull request as ready for review January 12, 2024 16:46
@jpwilliams jpwilliams merged commit a452cf1 into main Jan 12, 2024
34 of 35 checks passed
@jpwilliams jpwilliams deleted the feat/invoke-app-id branch January 12, 2024 17:10
jpwilliams pushed a commit that referenced this pull request Jan 12, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## inngest@3.10.0

### Minor Changes

- [#449](#449)
[`a452cf1`](a452cf1)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Add
`referenceFunction()`, allowing easier, typed invocation of functions
across apps and languages

- [#459](#459)
[`eec41d2`](eec41d2)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Add new
`Inngest.Any` and `InngestFunction.Any` type helpers

### Patch Changes

- [#460](#460)
[`a225206`](a225206)
Thanks [@MonsterDeveloper](https://github.com/MonsterDeveloper)! - Add
exports for `FinishedEventPayload` and `Context` types to fix a
TypeScript error when using Inngest in projects with `composite` setting
in `tsconfig`.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬆️ improvement Performance, reliability, or usability improvements 📦 inngest Affects the `inngest` package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants