Expression produces a union type that is too complex to represent.

whilefoo commented 6 months ago

We need to fix a Typescript issue: Expression produces a union type that is too complex to represent. I've tried multiple things:

function only accepting context of certain events

export async function getDefaultBranch(context: GitHubContext<"repository_dispatch">, owner: string, repository: string)

making the function generic

export async function getDefaultBranch<T extends EmitterWebhookEventName>(context: GitHubContext<T>, owner: string, repository: string)

casting a Context<"repository_dispatch"> to Context - it seems to work in my IDE but fails in CI but it's also a real solution
```
const defaultBranch = await getDefaultBranch(context as GitHubContext, nextPlugin.plugin.owner, nextPlugin.plugin.repo);
```

I've also got this problem when developing assistive-pricing plugin. I was trying to create typeguards like this:

export function isIssueCommentCreatedEvent(context: Context): context is Context<"issue_comment.created"> {
  return context.eventName === "issue_comment.created";
}

context is Context<"issue_comment.created"> triggers the error

0x4007 commented 6 months ago

@FernandVEYRIER you said you wanted to try your hand at this. Both whilefoo and I gave it our attempts but no luck. If you can't figure it out we will probably need to proceed with the method I proposed in our private messages, which would make the code not elegant and add a lot of bloat.

gentlementlegen commented 6 months ago

To follow up on the research, I tried:

updating TS version to the latest
downgrading TS version < 5.x
change rules in the tsconfig to use esnext
use yarn / npm instead of bun

and none of this was successful either. Every code change attempt led me to what @whilefoo has already covered. There is a very few results about this error, and it seems to be more of a TypeScript limitation that an actual code issue. Maybe we could also consider just silencing the error, as the IDE properly resolves the types, and it would be quite a loss to not have proper auto complete there (works fine in Webstorm as well).

There seem to be more errors on the typings as well, c.f. screenshot

0x4007 commented 6 months ago

I think we should type guard every webhook event. Then we can have auto completion.

This means a dedicated function for every webhook event (there's over 50)

We could start by trying the generic event first ex: issues_comment instead of the specific event ex: issues_comment.created

gentlementlegen commented 6 months ago

Well that's the thing, we already have access to full auto-completion, just TypeScript compiler doesn't understand it, so is it really worth it to spend all that time rewriting everything?

gentlementlegen commented 6 months ago

Another approach that works, and keeps a very similar design:

import { EmitterWebhookEvent as WebhookEvent, EmitterWebhookEventName as WebhookEventName } from "@octokit/webhooks";
import { customOctokit } from "./github-client";
import { GitHubEventHandler } from "./github-event-handler";

export class GitHubContext {
  public key: WebhookEventName;
  public name: WebhookEventName;
  public id: string;
  private readonly _payload: WebhookEvent<typeof this.key>["payload"];
  public octokit: InstanceType<typeof customOctokit>;
  public eventHandler: InstanceType<typeof GitHubEventHandler>;

  constructor(eventHandler: InstanceType<typeof GitHubEventHandler>, event: WebhookEvent, octokit: InstanceType<typeof customOctokit>) {
    this.eventHandler = eventHandler;
    this.name = event.name;
    this.id = event.id;
    this._payload = event.payload;
    if ("action" in this._payload) {
      this.key = `${this.name}.${this._payload.action}` as WebhookEventName;
    } else {
      this.key = this.name;
    }
    this.octokit = octokit;
  }

   getPayload<T extends WebhookEventName>(): WebhookEvent<T>["payload"] {
    return this._payload;
  }
}

export type SimplifiedContext = Omit<GitHubContext, keyof WebhookEventName>;

This does not throw complexity errors. It changes the use from

context: GitHubContext<"repository_dispatch">
context.payload
// To
context: GitHubContext
context.getPayload<"repository_dispatch">()

I don't know if this is viable for our design.

0x4007 commented 6 months ago

I assume it's fine but @whilefoo would know best.

gitcoindev commented 6 months ago

I spent a few hours on this being curious about the root cause and in the end found out this error is not so scary:

https://github.com/probot/probot/issues/1680 https://github.com/probot/probot/issues/1968 https://github.com/probot/probot/discussions/1966

Therefore, this is a known issue and the only workaround so far has been to narrow down context type or use context payload only.

Basically there are several options to avoid this Typescript compiler error:

1) Cast context argument to 'any' a.k.a. I am giving you noodles or sushi for dinner, you have to consume it. Example: Microsoft's fluentui https://github.com/microsoft/fluentui/blob/5e690201797c6b1ce34ac59638fd4a77a0f5a2b3/packages/fluentui/react-northstar-emotion-renderer/src/invokeKeyframes.ts#L50

2) Narrow down types or pass only payload of certain type. I am close to conclude that it complains about incompatibility between "repository_dispatch" vs "workflow_dispatch" context, so this would need need to be handled.

3) Add ts-ignore, an example https://github.com/taiga-family/argus/blob/d32d3f7d1d62d34c1e404e2fea24b94f1e7fcf51/src/selectors/workflow-run.selectors.ts#L59

4) Use a wrapper around context, example https://github.com/advanced-security/probot-security-alerts/blob/e7c7f636ab5593b33e79327be1b1589b44bdf13a/src/events/approvingTeam.ts#L6

0x4007 commented 6 months ago

The kernel doesn't use probot. Its a ground-up implementation to cut the bloat and work in Cloudflare Workers.

casting as any is a bad idea. It kills the point of type safety.
yes I suggested we typeguard at the webhook event handler function level. This will introduce a lot of type guards (one for every webhook type) but this is the only viable way I see to handle this with type safety.
refer to 1
we don't use probot so not sure if context is applicable here.

whilefoo commented 6 months ago

Even typeguards don't work. For example:

export function isRepositoryDispatchEvent(context: GitHubContext): context is GitHubContext<"repository_dispatch"> {
  return context.key === "repository_dispatch";
}

export async function repositoryDispatch(context: GitHubContext) {
  if (!isRepositoryDispatchEvent(context)) {
    console.error("Wrong event type");
    return;
  }
  console.log(context);
}

console.log(context) triggers the error

0x4007 commented 6 months ago

Maybe, somehow generate simplified types based on the corresponding real type and cast to use those?

Keyrxng commented 4 months ago

Spent an hour or so on this and the below approach solves things as far as I can tell, unsure how relevant this still is tho considering it's age but I enjoy a TS challenge me

Plugin side:

loose auto-complete stops string is not assignable to <union type>

basically plucks the events matching only those keys Context generic accepts only those events as T and then grabs the matching event with TU then extract payload via TU["payload"]

export type SupportedEventsU = "issues.labeled"
 | "issues.unlabeled"
 | "label.edited"
 | "issues.reopened"
 | "push"
 | "issue_comment.created"
 | (string & {}); // this allows for loose auto-complete 

export type SupportedEvents = {
  [K in SupportedEventsU]: K extends WebhookEventName ? WebhookEvent<K> : never;
};

export interface Context<T extends SupportedEventsU = SupportedEventsU, TU extends SupportedEvents[T] = SupportedEvents[T]> {
  eventName: T;
  payload: TU["payload"];
  ...
  }

Kernel side:

The errors kernel side seem to have been resolved/worked-around, but from what I can tell this resolves it kernel side too trying against repo_dispatch specifically and checking whether TS IDE errors were thrown for the rest.

I updated below, GithubContext and turned handleEvent and tryCatchWrapper into generics using SupportedEvents[T] and where needed assigning T using SupportedEvents<typeof event["name"]>

export type SupportedEvents<T extends EmitterWebhookEventName = EmitterWebhookEventName> = {
  [K in EmitterWebhookEventName]: T extends K ? EmitterWebhookEvent<T> : never;
};

export class DelegatedComputeInputs<T extends keyof SupportedEvents = keyof SupportedEvents, TU extends SupportedEvents[T] = SupportedEvents[T]> {
  public stateId: string;
  public eventName: T;
  public eventPayload: TU["payload"];

0x4007 commented 3 months ago

Even typeguards don't work. For example:

export function isRepositoryDispatchEvent(context: GitHubContext): context is GitHubContext<"repository_dispatch"> {
  return context.key === "repository_dispatch";
}

export async function repositoryDispatch(context: GitHubContext) {
  if (!isRepositoryDispatchEvent(context)) {
    console.error("Wrong event type");
    return;
  }
  console.log(context);
}

console.log(context) triggers the error

I am assuming that we need to typeguard like a whitelist. Basically we can not have an else conditional block for "everything else." We can only have if blocks with the type-guards we are supporting @whilefoo

ubiquibot[bot] commented 3 months ago

! No price label has been set. Skipping permit generation.

ubiquity-os-main[bot] commented 3 months ago

! No price label has been set. Skipping permit generation.

Keyrxng commented 3 months ago

ayyy I'm two for two with these tricky type tasks now

there was no price label set for the last one either, could this be priced and permit generated?

gitcoindev commented 3 months ago

ayyy I'm two for two with these tricky type tasks now

there was no price label set for the last one either, could this be priced and permit generated?

I think this is a reasonable request. I looked into this before and confirm it was tricky to solve.

0x4007 commented 3 months ago

! No price label has been set. Skipping permit generation.

The header is diff not text @gentlementlegen

0x4007 commented 3 months ago

ayyy I'm two for two with these tricky type tasks now

there was no price label set for the last one either, could this be priced and permit generated?

Sure although you should write the solution here so we can include it in the spec before you complete the task.

Then we can provide a time estimate and price it.

What time label are we adding? How long did it take you?

Keyrxng commented 3 months ago

There is a larger explanation in the PR I think covers things better but here is the boiled down solution:

public payload: {
    [K in TSupportedEvents]: K extends WebhookEventName ? WebhookEvent<K> : never;
  }[TSupportedEvents]["payload"];

A mapped type to help with the heavy lifting by creating a more efficient path for TS to find the underlying type as opposed to it trying to infer all combinations of the variance in payload between events when using WebhookEvent<T> (when WebhookEvent itself is a complicated type). Now it accesses a straightforward type that returns the baseWebhookEvent for any given T which is expected to always be a WebhookEventName.

This one took me about an hour the first one took a couple of hours but whatever is thought to be fair enough is fine.

ubiquibot[bot] commented 3 months ago

@Keyrxng the deadline is at 2024-06-11T20:42:33.608Z

ubiquibot[bot] commented 3 months ago

+ Evaluating results. Please wait...

ubiquity-os-main[bot] commented 3 months ago

[ 24.5865 WXDAI ]

@0x4007

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	8	22.1385
Review	Comment	1	2.448

Conversation Incentives

Comment	Formatting	Relevance	Reward
![image](https://github.com/ubiquity/ubiquibot-kernel/assets/497…	5.6 content: p: count: 56 score: 1 img: count: 1 score: 0 wordValue: 0.1 formattingMultiplier: 1	0.78	4.368
I think we should type guard every webhook event. Then we can ha…	4.7 content: p: count: 45 score: 1 code: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.67	3.149
I assume it's fine but @whilefoo would know best.	0.9 content: p: count: 9 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.655	0.5895
The kernel doesn't use probot. Its a ground-up implementation to…	9.3 content: p: count: 91 score: 1 code: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.73	6.789
Maybe, somehow generate simplified types based on the correspond…	1.6 content: p: count: 16 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.64	1.024
I am assuming that we need to typeguard like a whitelist. Basica…	3.8 content: p: count: 36 score: 1 code: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.69	2.622
The header is `diff` not `text` @gentlementlegen	0.9 content: p: count: 7 score: 1 code: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.7	0.63
Sure although you should write the solution here so we can inclu…	4.3 content: p: count: 43 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.69	2.967
I am skeptical here because the whole team tried and nobody coul…	3.4 content: p: count: 34 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.72	2.448

[ 30.376 WXDAI ]

@gentlementlegen

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	30.376

Conversation Incentives

Comment	Formatting	Relevance	Reward
To follow up on the research, I tried: - updating TS version to …	13.1 content: p: count: 130 score: 1 code: count: 1 score: 1 img: count: 1 score: 0 wordValue: 0.1 formattingMultiplier: 1	0.81	10.611
Well that's the thing, we already have access to full auto-compl…	3 content: p: count: 30 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.67	2.01
Another approach that works, and keeps a very similar design: &#…	26.5 content: p: count: 148 score: 1 code: count: 117 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.67	17.755

Comment

Formatting

Relevance

Reward

To follow up on the research, I tried: - updating TS version to …

13.1

content:  p:    count: 130    score: 1  code:    count: 1    score: 1  img:    count: 1    score: 0wordValue: 0.1formattingMultiplier: 1

0.81

10.611

Well that's the thing, we already have access to full auto-compl…

3

content:  p:    count: 30    score: 1wordValue: 0.1formattingMultiplier: 1

0.67

2.01

Another approach that works, and keeps a very similar design: &#…

26.5

content:  p:    count: 148    score: 1  code:    count: 117    score: 1wordValue: 0.1formattingMultiplier: 1

0.67

17.755

[ 13.779 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	2	13.779

Conversation Incentives

Comment	Formatting	Relevance	Reward
I spent a few hours on this being curious about the root cause a…	13.6 content: p: count: 136 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.9	12.24
I think this is a reasonable request. I looked into this before …	1.9 content: p: count: 19 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.81	1.539

[ 37.9745 WXDAI ]

@whilefoo

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	13.0835
Issue	Comment	1	11.36
Review	Comment	2	13.531

Conversation Incentives

Comment	Formatting	Relevance	Reward
We need to fix a Typescript [issue](https://github.com/ubiquity/…	19.1 content: p: count: 129 score: 1 a: count: 2 score: 1 code: count: 60 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.685	13.0835
Even typeguards don't work. For example: ```js expor…	14.2 content: p: count: 40 score: 1 code: count: 31 score: 1 wordValue: 0.2 formattingMultiplier: 1	0.8	11.36
This looks promising, I tried it and it works. I can also see wh…	13.7 content: p: count: 96 score: 1 code: count: 41 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.95	13.015
@Keyrxng you can make the change	0.6 content: p: count: 6 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.86	0.516

[ 409.118 WXDAI ]

@Keyrxng

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1	200
Issue	Comment	3	0
Review	Comment	6	209.118

Conversation Incentives

Comment	Formatting	Relevance	Reward
Spent an hour or so on this and the below approach solves things…	0 content: p: count: 242 score: 1 code: count: 125 score: 1 wordValue: 0 formattingMultiplier: 0	0.69	-
ayyy I'm two for two with these tricky type tasks now there was …	0 content: p: count: 29 score: 1 a: count: 2 score: 1 wordValue: 0 formattingMultiplier: 0	0.595	-
There is a larger explanation in the PR I think covers things be…	0 content: p: count: 131 score: 1 code: count: 21 score: 1 wordValue: 0 formattingMultiplier: 0	0.605	-
Resolves #31 Using the same type setup I've used in https://git…	0 content: p: count: 27 score: 1 wordValue: 0 formattingMultiplier: 0	0.505	-
Believe me so was I for this very same reason but I went through…	206 content: p: count: 375 score: 1 code: count: 140 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.65	133.9
I didn't really touch on why I think this resolves the underlyin…	99.2 content: p: count: 223 score: 1 code: count: 25 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.585	58.032
A much cleaner way to represent it for sure @whilefoo I wasn't …	13.2 content: p: count: 33 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.575	7.59
@whilefoo I'm unsure whether to make those changes or if you are…	14.4 content: p: count: 36 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.47	6.768
I think you commented in the same sec that I pushed the change 😂	5.6 content: p: count: 14 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.505	2.828

ubiquibot[bot] commented 3 months ago

[ 46.7 WXDAI ]

@0x4007

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	8	46.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
![image](https://github.com/ubiquity/ubiquibot-kernel/assets/49...	5.7	0.815	5.7
I think we should type guard every webhook event. Then we can ha...	6.7 code: count: 2 score: "2" words: 3	0.765	6.7
I assume it's fine but @whilefoo would know best. ...	1	0.66	1
The kernel doesn't use probot. Its a ground-up implementation to...	15 li: count: 4 score: "4" words: 74 code: count: 2 score: "2" words: 2	0.76	15
Maybe, somehow generate simplified types based on the correspond...	1.6	0.705	1.6
> Even typeguards don't work. For example: > > `...	7.7 code: count: 4 score: "4" words: 5	0.685	7.7
> ```text > ! No price label has been set. Ski...	3.7 code: count: 3 score: "3" words: 2	0.75	3.7
> ayyy I'm two for two with these tricky type tasks now > ...	5.3 a: count: 1 score: "1" words: 2	0.805	5.3

[ 38.7 WXDAI ]

@gentlementlegen

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	38.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
To follow up on the research, I tried: - updating TS version to...	17.6 li: count: 4 score: "4" words: 28 code: count: 1 score: "1" words: 1	0.84	17.6
Well that's the thing, we already have access to full auto-compl...	3.3	0.71	3.3
Another approach that works, and keeps a very similar design: &...	17.8 code: count: 2 score: "2" words: 0	0.685	17.8

[ 26.7 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	2	26.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
I spent a few hours on this being curious about the root cause a...	23.8 li: count: 4 score: "4" words: 0	0.685	23.8
> ayyy I'm two for two with these tricky type tasks now >...	2.9 a: count: 1 score: "1" words: 2	0.76	2.9

[ 453 WXDAI ]

@Keyrxng

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1	200
Issue	Comment	3	0
Issue	Comment	3	61.6
Review	Comment	5	95.7
Review	Comment	5	95.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
Spent an hour or so on this and the below approach solves things...	- code: count: 14 score: "0" words: 31	0.71	-
ayyy I'm two for two with these tricky type tasks now there w...	- a: count: 1 score: "0" words: 2	0.755	-
There is a larger explanation in the PR I think covers things be...	- code: count: 7 score: "0" words: 9	0.76	-
Spent an hour or so on this and the below approach solves things...	37.6 code: count: 14 score: "14" words: 31	0.71	37.6
ayyy I'm two for two with these tricky type tasks now there w...	4 a: count: 1 score: "1" words: 2	0.755	4
There is a larger explanation in the PR I think covers things be...	20 code: count: 7 score: "7" words: 9	0.76	20
> I am skeptical here because the whole team tried and nobody...	57.5 code: count: 21 score: "21" words: 54	-	57.5
I didn't really touch on why I think this resolves the underlyin...	29.4 code: count: 6 score: "6" words: 11	-	29.4
A much cleaner way to represent it for sure @whilefoo I wasn...	3.7	-	3.7
@whilefoo I'm unsure whether to make those changes or if you are...	3.8	-	3.8
I think you commented in the same sec that I pushed the change ?...	1.3	-	1.3
> I am skeptical here because the whole team tried and nobody...	57.5 code: count: 21 score: "21" words: 54	-	57.5
I didn't really touch on why I think this resolves the underlyin...	29.4 code: count: 6 score: "6" words: 11	-	29.4
A much cleaner way to represent it for sure @whilefoo I wasn...	3.7	-	3.7
@whilefoo I'm unsure whether to make those changes or if you are...	3.8	-	3.8
I think you commented in the same sec that I pushed the change ?...	1.3	-	1.3

[ 75.4 WXDAI ]

@whilefoo

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	39.8
Issue	Comment	1	10.8
Review	Comment	2	24.8

Conversation Incentives

Comment	Formatting	Relevance	Reward
We need to fix a Typescript [issue](https://github.com/ubiquity/...	39.8 a: count: 2 score: "2" words: 3 li: count: 3 score: "3" words: 42 code: count: 6 score: "6" words: 20 hr: count: 1 score: "1" words: 0	1	39.8
Even typeguards don't work. For example: ```js e...	10.8 code: count: 2 score: "2" words: 3	0.765	10.8
This looks promising, I tried it and it works. I can also see wh...	23.6 code: count: 2 score: "4" words: 1	-	23.6
@Keyrxng you can make the change...	1.2	-	1.2

ubiquity-os / ubiquity-os-kernel