Make EmitterWebhookEventName readonly

[elibarzilay]

What’s missing?

Methods like .on and .removeListener use event: E | E[], it would be nice to change it to event: E | readonly E[], since it's convenient to have an as const list of event names (which currently get a type error when passing it to an E[] argument).

Why?

Makes TS code better.

Alternatives you tried

const eventNames = [ ... ] as const;
const eventNamesSillyCopy = eventNames.slice(0);

[G-Rath]

We've actually already got a PR open for this, but the issue is it requires us to do some nasty looking casting or otherwise add additional runtime code that is effectively a no-op.

I wasn't aware about it causing an error if you pass in a const however

🤦 duh course you will, you're passing a non-mutable type into a mutable one

---

I'll try and have a look into that sometime in the next couple of days.

[elibarzilay]

@G-Rath, heh, I'm 95% sure that I got here by looking at the code that @jablko wrote which is likely how he got to the same place... (The code did a nasty ... as unknown as unknown as typeof eventNames[number] which is subtly wrong since it should have an extra [] around the RHS.)

And yes, the convenient thing about as const arrays is that you can use typeof a[number] to get a union of known values.

[G-Rath]

I agree it'd be nice, but like I said currently it's requiring a pretty ugly cast that I can see causing trouble on our side down the line, but I'll try and find some time to look into it.

For now you should be able to just do something like [...eventNames] to make an inline fresh mutable array to avoid the error.

[gr2m]

@G-Rath what can we do here right now? Is this issue blocked by something right now?

[G-Rath]

@gr2m I believe this was the blocker - we weren't all that happy with the unsafe casting that was required to ship this vs the minimal gain.

[gr2m]

would you suggest to close this issue then as won't fix? Or shall we leave it open and try again in future?

[jablko]

It's not clear why the casting is unsafe?

[gr2m]

I'd very much appreciate an explanation for

Makes TS code better.

Better how? And better for users of @octokit/webhooks, or just more elegant code, without an effect?

[elibarzilay]

1. See the slice hack at the top, where the problem is the need for a copy of the array just to get a non-readonly version of it.
2. We eventually went with the slight variation that @G-Rath mentioned.
3. A previous version didn't do a copy, but an unsafe cast instead.

Also, AFAICT, the casting done in @jablko's PR is safe since the input type itself requires a readonly input; and I don't think that there should be a downside for that.

I agree that it's ugly, but cases like ours show that not doing it is just pushing the same ugliness on users of this code... Assuming that bugs eventually get fixed, I think that it makes sense to use the patch (in the PR that you closed), with a comment to remove the hack once the underlying problem gets fixed.