🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

jablko · 2021-02-04T20:29:28Z

Can we eliminate get-webhook-payload-type-from-event.ts thanks to #435?

Replace EmitterEventWebhookPayloadMap[TName] with EventPayloadMap[TWebhookEvent] & { action: TAction }
Replace keyof EmitterEventWebhookPayloadMap with typeof emitterEventNames[number]

G-Rath · 2021-02-04T20:39:04Z

I had originally laid the types out like this (or similar to this), but decided against it in favor of how it is currently as it's easier to predict the behaviour (both in typings and in performance).

scripts/generate-types.ts

G-Rath · 2021-02-04T20:42:06Z

src/event-handler/receive.ts

-  EmitterEventWebhookPayloadMap[keyof EmitterEventWebhookPayloadMap],
-  { action: string }
+  EmitterWebhookEvent["payload"],
+  { action: any }


Suggested change

{ action: any }

{ action: string }

G-Rath · 2021-02-04T20:46:41Z

I've not checked out this out locally, but I feel like it's not preserving the mapping of the event + action to the specific type for that action. We're probably not got any tests covering this as there are some subtle differences in the events.

~~I'll push one up so we can see.~~

Nope it'll be fine because that's handled by the types for each payload being a union 🤦

G-Rath · 2021-02-04T20:53:15Z

src/types.ts

+  [K in Exclude<
+    typeof emitterEventNames[number],
+    "error"
+  >]: K extends `${infer TWebhookEvent}.${infer TAction}`


Can you please make this a "type function"? i.e.

type WithAction<...> = K extends ... // etc

G-Rath · 2021-02-04T20:55:09Z

src/types.ts


 export type EmitterWebhookEventMap = {
-  [K in keyof EmitterEventPayloadMap]: BaseWebhookEvent<K>;
+  [K in Exclude<
+    typeof emitterEventNames[number],


I think let's make this a type alias in the generated type:

export type EmitterEventName = typeof emitterEventNames[number];

G-Rath

I'm going to get started on a next branch, which'll likely include this change so going to hold off merging it for now.

G-Rath · 2021-02-05T19:15:05Z

Just FYI, as I mentioned in #444: this change causes massive performance problems in the current version of WebStorm - while TypeScript seems to be able to handle it fine, I don't think there's a huge gain to be had that makes it worth putting such a massive blocker on anyone using that IDE.

I've submitted an issue on YouTrack and pinged the team, so hopefully it'll get fixed promptly (but even once fixed we'd want to wait for the release, which'd be a few months later).

jablko · 2021-02-05T20:19:47Z

I wonder if removing EmitterWebhookEventMap/EmitterEventMap would make a difference? When you get a chance, would you mind seeing whether the latest revision makes a difference in WebStorm, one way or the other?

G-Rath · 2021-02-05T20:41:14Z

That seems to have fixed both this and the performance problems caused by inferring the impact of transform in #440 🎉

Currently I'm on the fence about shipping this because of the huge impact if I'm wrong, but def going to keep this patch around to apply it at a later date:

patch

Index: src/types.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/types.ts b/src/types.ts
--- a/src/types.ts	(revision 560ff73ef02453fd0a28705e042e8ad3538c43ef)
+++ b/src/types.ts	(date 1612557132345)
@@ -1,25 +1,33 @@
-import type { RequestError } from "@octokit/request-error";
-import type { EmitterEventWebhookPayloadMap } from "./generated/get-webhook-payload-type-from-event";
+import { RequestError } from '@octokit/request-error';
+import type {
+  EventPayloadMap,
+  Schema,
+  WebhookEventName,
+} from "@octokit/webhooks-definitions/schema";
+import type { EmitterEventName } from "./generated/webhook-names";
+
+export { EmitterEventName };
 
-export type EmitterWebhookEventMap = {
-  [K in keyof EmitterEventWebhookPayloadMap]: BaseWebhookEvent<K>;
-};
+type EmitterEventPayloadMap = { "*": Schema } & EventPayloadMap;
 
-export type EmitterWebhookEventName = keyof EmitterWebhookEventMap;
-export type EmitterWebhookEvent = EmitterWebhookEventMap[EmitterWebhookEventName];
+export type EmitterWebhookEventName = EmitterEventName;
 
-type ToWebhookEvent<
-  TEmitterEvent extends string
-> = TEmitterEvent extends `${infer TWebhookEvent}.${string}`
-  ? TWebhookEvent
-  : TEmitterEvent;
+export type EmitterWebhookEvent<
+  TEmitterEvent = WebhookEventName
+> = TEmitterEvent extends `${infer TWebhookEvent}.${infer TAction}`
+  ? BaseWebhookEvent<Extract<TWebhookEvent, keyof EmitterEventPayloadMap>> & {
+      payload: { action: TAction };
+    }
+  : BaseWebhookEvent<Extract<TEmitterEvent, keyof EmitterEventPayloadMap>>;
 
-interface BaseWebhookEvent<
-  TName extends EmitterWebhookEventName = EmitterWebhookEventName
-> {
+type EmitterEvent<TEmitterEvent> = TEmitterEvent extends "error"
+  ? WebhookEventHandlerError
+  : EmitterWebhookEvent<TEmitterEvent>;
+
+interface BaseWebhookEvent<TName extends keyof EmitterEventPayloadMap> {
   id: string;
-  name: ToWebhookEvent<TName>;
-  payload: EmitterEventWebhookPayloadMap[TName];
+  name: TName;
+  payload: EmitterEventPayloadMap[TName];
 }
 
 export interface Options<
@@ -39,15 +47,9 @@
 // type MaybeArray<T> = T | T[];
 
 export type HandlerFunction<
-  TName extends EmitterWebhookEventName | EmitterWebhookEventName[],
+  TName extends EmitterEventName | EmitterEventName[],
   TTransformed
-> = (
-  event: EmitterWebhookEventMap[Extract<
-    EmitterWebhookEventName,
-    EnsureArray<TName>[number]
-  >] &
-    TTransformed
-) => any;
+> = (event: EmitterEvent<EnsureArray<TName>[number]> & TTransformed) => any;
 
 type Hooks = {
   [key: string]: Function[];
Index: test/typescript-validate.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript-validate.ts b/test/typescript-validate.ts
--- a/test/typescript-validate.ts	(revision 560ff73ef02453fd0a28705e042e8ad3538c43ef)
+++ b/test/typescript-validate.ts	(date 1612557132359)
@@ -9,7 +9,7 @@
   WebhookError,
 } from "../src/index";
 import { createServer } from "http";
-import { HandlerFunction, EmitterWebhookEventName } from "../src/types";
+import { EmitterEventName, HandlerFunction } from '../src/types';
 
 // ************************************************************
 // THIS CODE IS NOT EXECUTED. IT IS FOR TYPECHECKING ONLY
@@ -23,7 +23,7 @@
   }
 };
 
-declare const on: <E extends EmitterWebhookEventName>(
+declare const on: <E extends EmitterEventName>(
   name: E | E[],
   callback: HandlerFunction<E, unknown>
 ) => void;
Index: src/generated/webhook-names.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/generated/webhook-names.ts b/src/generated/webhook-names.ts
--- a/src/generated/webhook-names.ts	(revision 560ff73ef02453fd0a28705e042e8ad3538c43ef)
+++ b/src/generated/webhook-names.ts	(date 1612556741606)
@@ -206,4 +206,6 @@
   "workflow_run",
   "workflow_run.completed",
   "workflow_run.requested",
-];
+] as const;
+
+export type EmitterEventName = typeof emitterEventNames[number];

In saying that, I think it probably has fixed the issue and I'm just being paranoid 😅
I'll let things sit for a bit so that the WebStorm team has a chance to look into it, and to give myself some time to put the types through their paces a bit more to confirm, but I think this should be fine to ship in the near future! 🎉

(/cc @anstarovoyt, since it might help you pin down where the performance issue is)

jablko · 2021-02-05T21:11:50Z

Awesome, thanks! 👍

oscard0m · 2021-02-07T22:38:09Z

I've submitted an issue on YouTrack and pinged the team, so hopefully it'll get fixed promptly (but even once fixed we'd want to wait for the release, which'd be a few months later).

Could you link please the issue on YouTrack, please? :) 🙏🏽

G-Rath · 2021-02-07T22:52:15Z

https://youtrack.jetbrains.com/issue/WEB-49483

github-actions · 2021-02-11T22:14:37Z

🎉 This issue has been resolved in version 8.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

G-Rath reviewed Feb 4, 2021

View reviewed changes

scripts/generate-types.ts Show resolved Hide resolved

G-Rath reviewed Feb 4, 2021

View reviewed changes

jablko force-pushed the patch-2 branch from 08e5f69 to 2da76de Compare February 4, 2021 21:57

G-Rath requested changes Feb 5, 2021

View reviewed changes

G-Rath mentioned this pull request Feb 5, 2021

v8.0.0 #444

Merged

jablko added 2 commits February 5, 2021 08:06

refactor(types): replace EmitterEventWebhookPayloadMap

30be192

Apply suggestions from code review

ab684f2

jablko force-pushed the patch-2 branch from 2da76de to ab684f2 Compare February 5, 2021 15:21

jablko changed the title ~~refactor: replace EmitterEventWebhookPayloadMap~~ refactor(types): replace EmitterEventWebhookPayloadMap Feb 5, 2021

jablko added 3 commits February 5, 2021 12:31

fix(types): .onAny() even.name

2bf3aef

fix(types): .receive() and .verifyAndReceive() consistency

cd0a76f

Improve performance in WebStorm

501e799

Merge branch 'master' into patch-2

af85717

G-Rath changed the title ~~refactor(types): replace EmitterEventWebhookPayloadMap~~ 🚧 refactor(types): replace EmitterEventWebhookPayloadMap Feb 7, 2021

wolfy1339 added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR typescript Relevant to TypeScript users only labels Feb 7, 2021

gr2m closed this in #444 Feb 8, 2021

github-actions bot added the released label Feb 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

jablko commented Feb 4, 2021

G-Rath commented Feb 4, 2021 •

edited

Loading

G-Rath Feb 4, 2021

G-Rath commented Feb 4, 2021 •

edited

Loading

G-Rath Feb 4, 2021

G-Rath Feb 4, 2021

G-Rath left a comment •

edited

Loading

G-Rath commented Feb 5, 2021 •

edited

Loading

jablko commented Feb 5, 2021

G-Rath commented Feb 5, 2021 •

edited

Loading

jablko commented Feb 5, 2021

oscard0m commented Feb 7, 2021

G-Rath commented Feb 7, 2021

github-actions bot commented Feb 11, 2021

🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

Conversation

jablko commented Feb 4, 2021

G-Rath commented Feb 4, 2021 • edited Loading

G-Rath Feb 4, 2021

Choose a reason for hiding this comment

G-Rath commented Feb 4, 2021 • edited Loading

G-Rath Feb 4, 2021

Choose a reason for hiding this comment

G-Rath Feb 4, 2021

Choose a reason for hiding this comment

G-Rath left a comment • edited Loading

Choose a reason for hiding this comment

G-Rath commented Feb 5, 2021 • edited Loading

jablko commented Feb 5, 2021

G-Rath commented Feb 5, 2021 • edited Loading

jablko commented Feb 5, 2021

oscard0m commented Feb 7, 2021

G-Rath commented Feb 7, 2021

github-actions bot commented Feb 11, 2021

G-Rath commented Feb 4, 2021 •

edited

Loading

G-Rath commented Feb 4, 2021 •

edited

Loading

G-Rath left a comment •

edited

Loading

G-Rath commented Feb 5, 2021 •

edited

Loading

G-Rath commented Feb 5, 2021 •

edited

Loading