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

Type Parameter Issues for Context #1968

Comments

@wolfy1339
Copy link
Contributor

Discussed in #1966

Originally posted by tmercswims February 5, 2024
This discussion is essentially the same problem as #1680, so I'll try to summarize and simplify as much as I can. The solutions given on that issue are not working for me, and rather than resurrect a closed issue I figured I'd start fresh.

The bot I'm making is in TypeScript. As such, I want/need to use type parameters on Context in order for it to have the properties that it should have on its payload. Typically that looks something like this:

async function onPullRequestLabeled(context: Context<'pull_request.labeled'>): Promise<void> {
    // do something
};

The problem I'm facing is whenever I try to do anything else with the Context which implies some difference in the types, even if that difference "should" be allowed. Consider this situation:

// attached to a webhook event
async function onPullRequestLabeled(context: Context<'pull_request.labeled'>): Promise<void> {
    await checkReadyToMerge(context);
};

// attached to a webhook event
async function onPullRequestReviewed(context: Context<'pull_request_review.submitted'>): Promise<void> {
    await checkReadyToMerge(context);
};

// not attached to a webhook event
async function checkReadyToMerge(context: Context<'pull_request.labeled' | 'pull_request_review.submitted'>): Promise<boolean> {
    // do something
};

In this case, I care about two different webhook events, which aren't the same but are similar enough that I can do the same processing to both of them. I've set the types up to support that, as you can see.

However, this does not work. TypeScript gets mad on whichever call of checkReadyToMerge() it encounters first during complication, with the following error:

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

If I change things so that type parameters of everything here match exactly, for example make them all Context<'pull_request.labeled'>, then that error does not occur. But that isn't what I want, I want onPullRequestReviewed to take a context of type Context<'pull_request_review.submitted'>. This happens if there is a mismatch in the type parameter of any two Context types, assuming that one calls the other or something like that. It even happens if one of them's type parameter is omitted entirely, or is set to any.

I have also tried unioning on multiple Contexts, each with a single, different type parameter, (Context<'pull_request_review.submitted'> | Context<'pull_request.labeled'> instead) but the same error occurs when compiling.

The same problem also happens if I'm using sub-types of a webhook, like one handler for pull_request.opened and another for pull_request.labeled which both call a helper function which is set up to take Context<'pull_request'>. I'd expect either of pull_request.opened and pull_request.labeled to satisfy the type requirement for just pull_request, and maybe it would, but TypeScript complains with the same complexity error before I can know.

I can always get around these issues by strategically casting things to any, but I'd really rather not do that if I can at all help it.

I'm looking for any advice on what to do here. Thank you!

Versions of relevant things:

  • @octokit/webooks: 12.1.0 (latest)
  • @octokit/webhooks-types: 7.3.2 (latest)
  • probot: 13.0.2 (latest)
  • typescript: 5.3.3 (latest)
@wolfy1339
Copy link
Contributor Author

wolfy1339 commented Feb 6, 2024

That's the only solution I have for you right now, sorry.

The type issues don't happen with the payload nor Octokit, and I don't think it's the logger as that is the same method regardless of the event.

I'm thinking it's probably the convenience methods that are causing this, as they are heavily tied to the input event names when instantiating the Context class

Originally posted by @wolfy1339 in #1966 (reply in thread)

GitHub
This discussion is essentially the same problem as #1680, so I'll try to summarize and simplify as much as I can. The solutions given on that issue are not working for me, and rather than resurrect...

@ajschmidt8
Copy link
Contributor

I suffered from this issue for a while.

I was able to work around it by separating the context and payload arguments.

So instead of:

// not attached to a webhook event
async function checkReadyToMerge(context: Context<'pull_request.labeled' | 'pull_request_review.submitted'>): Promise<boolean> {
    // do something
};

try this:

// not attached to a webhook event
async function checkReadyToMerge(context: Context, payload: Context<'pull_request.labeled' | 'pull_request_review.submitted'>["payload"]): Promise<boolean> {
    // do something
};

Hope this helps.

wolfy1339 added a commit that referenced this issue Aug 3, 2024
Fixes #1968

> error TS2590: Expression produces a union type that is too complex to represent.
@Uzlopak Uzlopak closed this as completed in 23f6eab Aug 6, 2024
Copy link

github-actions bot commented Aug 6, 2024

🎉 This issue has been resolved in version 13.3.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment