inngest / inngest-js

The developer platform for easily building reliable workflows with zero infrastructure for TypeScript & JavaScript
https://www.inngest.com/
GNU General Public License v3.0
414 stars 41 forks source link

[BUG] Inngest Client has TS error `Parsing error: Property or signature expected` after upgrade to 3.21.1 #658

Closed joekrall closed 2 months ago

joekrall commented 2 months ago

Describe the bug The exported Inngest client instance has the type error: Parsing error: Property or signature expected.

To Reproduce Update to 3.21.1 and run the TypeScript server against client.ts.

Expected behavior No errors.

Code snippets / Logs / Screenshots Lightly-changed instance:

export const inngest = new Inngest({
  id: 'my-next-app',
  schemas: new EventSchemas().fromRecord<Events>(),
  middleware: [prismaMiddleware, loggingMiddleware],
})

System info (please complete the following information):

Additional Running "typescript": "5.5.3".

linear[bot] commented 2 months ago

INN-3320 [BUG] Inngest Client has TS error `Parsing error: Property or signature expected` after upgrade to 3.21.1

jpwilliams commented 2 months ago

Hi @joekrall!

Thanks for the report! Could I check a few more details?

joekrall commented 2 months ago
jpwilliams commented 2 months ago

Thank you! Hmm not yet able to reproduce.

Could I see the contents of your tsconfig.json?

joekrall commented 2 months ago
{
  "compilerOptions": {
    "allowJs": true,
    "baseUrl": "./",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "incremental": true,
    "isolatedModules": true,
    "jsx": "preserve",
    "lib": ["dom", "dom.iterable", "esnext"],
    "module": "esnext",
    "moduleResolution": "node",
    "noEmit": true,
    "paths": {
      "@/*": ["src/*"]
    },
    "plugins": [
      {
        "name": "next"
      }
    ],
    "resolveJsonModule": true,
    "skipLibCheck": true,
    "strict": true,
    "target": "es2017"
  },
  "exclude": ["node_modules"],
  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts"]
}
jpwilliams commented 2 months ago

Thank you!

Hmm still no luck! Does this happen for you in a fresh project with inngest? A minimal reproduction repo or even something on CodeSandbox/StackBlitz would be fantastic!

Have you tried blowing node_modules away and reinstalling everything too? I highly (highly) doubt that's it, but obtuse errors like this can sometimes surface with a corrupt install.

itzsaga commented 2 months ago

@jpwilliams hopping in here. Coworker of @joekrall. I believe this one is relatively straightforward. The build error is:

./src/inngest/client.ts:330:42
Type error: Type 'Events' does not satisfy the constraint 'StandardEventSchemas'.
  Property ''app/sendEmail'' is incompatible with index signature.
    Type '{ data: MailDataRequired | MailDataRequired[]; }' is not assignable to type 'StandardEventSchema'.
      Types of property 'data' are incompatible.
        Type 'MailDataRequired | MailDataRequired[]' is not assignable to type 'Record<string, unknown> | undefined'.
          Type 'MailData & { text: string; }' is not assignable to type 'Record<string, unknown> | undefined'.
            Type 'MailData & { text: string; }' is not assignable to type 'Record<string, unknown>'.
              Index signature for type 'string' is missing in type 'MailData & { text: string; }'.

  328 | export const inngest = new Inngest({
  329 |   id: 'gratia-next-app',
> 330 |   schemas: new EventSchemas().fromRecord<Events>(),
      |                                          ^
  331 |   middleware: [prismaMiddleware, loggingMiddleware],
  332 | })
  333 |
 ELIFECYCLE  Command failed with exit code 1.

3.21.1 intentionally changed the typing.

Do not allow objectish [] for an event's data when providing schemas

I believe this change should be a major version as it introduces an incompatible API change to people like us and others. This is not a patch that fixes a bug in a backwards compatible manner.

I suspect something similar is happening in #659 as the type was changed from any to unknown.

jpwilliams commented 2 months ago

Hi @itzsaga!

Thanks - ah, that stack trace makes much more sense.

I'm sorry that the change broke your build! As you say, usually these would be major changes, but as this is helping you fix a runtime bug we wanted to release it to folks now.

If your case, if you did actually send event.data as MailDataRequired[], the event send would fail at runtime and throw an error; while the patch does break your build, it's front-loading the error to ensure you can solve it now before it inevitably happens in production.

Typing event.data is as just MailDataRequired should fix the build issue.

I'm also keen to know what we can do better here - we take breaking changes very seriously and want to interrupt your workflow as little as possible, but an exception was made here to protect against an inevitable runtime error. Any disruption at all can be improved, though!

itzsaga commented 2 months ago

Thanks @jpwilliams,

My feedback would be, don't make exceptions or the versions lose their purpose. Cut a major and indicate in the release notes why, what could break, and link to documentation (probably around batching/fan out) to remediate instances where people have array types being sent as values for the data key.

You indicated the exception was made to get the fix out quickly. It seems like majors must take more effort to release then. Based on our code the API contract has been like this for a LONG time. Thus, in my opinion, this isn't a "break all the rules" to get it out as fast as humanly possible bug fix.

This is the second non-major that has changed how things work in a non-backwards compatible manner and affected us. The first one I had to get on a call with Tony about back in March and changed runtime behavior in a way that caused us customer issues.

All that aside, keep up the great work! We truly enjoy the Inngest product offering and the team responds quickly to things like this.

itzsaga commented 2 months ago

Typing event.data is as just MailDataRequired should fix the build issue.

This did not fix it:

./src/inngest/client.ts:329:42
Type error: Type 'Events' does not satisfy the constraint 'StandardEventSchemas'.
  Property ''app/sendEmail'' is incompatible with index signature.
    Type '{ data: MailDataRequired; }' is not assignable to type 'StandardEventSchema'.
      Types of property 'data' are incompatible.
        Type 'MailDataRequired' is not assignable to type 'Record<string, unknown> | undefined'.
          Type 'MailData & { text: string; }' is not assignable to type 'Record<string, unknown> | undefined'.
            Type 'MailData & { text: string; }' is not assignable to type 'Record<string, unknown>'.
              Index signature for type 'string' is missing in type 'MailData & { text: string; }'.

  327 | export const inngest = new Inngest({
  328 |   id: 'gratia-next-app',
> 329 |   schemas: new EventSchemas().fromRecord<Events>(),
      |                                          ^
  330 |   middleware: [prismaMiddleware, loggingMiddleware],
  331 | })
  332 |
 ELIFECYCLE  Command failed with exit code 1.

We're creating our Events type following the documentation here.

For deeper context, this is a function that, probably obviously, sends an e-mail. We're using the type from the SendGrid SDK. The underlying MailDataRequired type looks like this:

export interface MailData {
  to?: EmailData|EmailData[],
  cc?: EmailData|EmailData[],
  bcc?: EmailData|EmailData[],

  from: EmailData,
  replyTo?: EmailData,

  sendAt?: number,

  subject?: string,
  text?: string,
  html?: string,
  content?: MailContent[],
  templateId?: string,

  personalizations?: PersonalizationData[],
  attachments?: AttachmentData[],

  ipPoolName?: string,
  batchId?: string,

  sections?: { [key: string]: string },
  headers?: { [key: string]: string },

  categories?: string[],
  category?: string,

  customArgs?: { [key: string]: any },
  asm?: ASMOptions,

  mailSettings?: MailSettings,
  trackingSettings?: TrackingSettings,

  substitutions?: { [key: string]: string },
  substitutionWrappers?: string[],

  isMultiple?: boolean,
  dynamicTemplateData?: { [key: string]: any },

  hideWarnings?: boolean,

  replyToList?: EmailJSON | EmailJSON[],
}

export type MailDataRequired = MailData & (
    { text: string } | { html: string } | { templateId: string } | { content: MailContent[] & { 0: MailContent } });
jpwilliams commented 2 months ago

@itzsaga: This did not fix it

Ah, we might be backed into a corner with the types here anyway. 🫠

Record<string, unknown> needs to know how to index it correctly, meaning an interface doesn't satisfy it without either:

Both of those are terrible solutions.

My guess is that you're already using 3.21.0 to work around this, but I'll revert and ship as 3.21.2.


As for your feedback, thank you and apologies once again that this impacted you. A large reason for resisting majors with fixes like these is to avoid having users scattered across major versions and needing to backport fixes to many different versions. It may be that the wrong decision was made here though.

We'll discuss this internally - we don't want these issues happening!