v8.0.0 #444

G-Rath · 2021-02-05T04:22:45Z

I've done this all in one PR as there's some overlapping changes and I think it's just easier to review as a whole. There are a few changes I'll look to pull out (specifically the test & typo fixings).

I think this is pretty much good to go, but going to make it a draft for now all the same.

Closes #439
Closes #440
Closes #441
Closes #443
Closes #445
Closes #447

I've deliberately not applied the main change from #441 as while it's nice not to have the file, it causes massive performance issues in my IDE which I think are not worth the trade off since it'll occur for other WebStorm users.

There's also a performance problem when having to infer the generic for TTransformed, but that can be worked around by just providing the generic explicitly so so long as I don't have typescript-validate open I can still work on the project without issue 😅

I have opened a ticket in YouTrack, and will hopefully be able to get that resolved.

Also, we could revert the type refactoring on 7.x.x so that probot and co wouldn't have as a sad time, and then just include that refactor in this release.

View rendered .github/ISSUE_TEMPLATE/04_feature_request.md
View rendered README.md

BREAKING CHANGE: passing `error` as an event name is no longer supported

BREAKING CHANGE: passing `*` as an event name is no longer supported

BREAKING CHANGE: payload types have been renamed and refactored

G-Rath · 2021-02-05T04:31:06Z

@anstarovoyt @prigara the performance problems are pretty massive & showstopping, and it'd be very easy for a package to ship with types that'd grind things to a halt so it'd be great to get this resolved quickly.

Happy to help anyway I can :)

gr2m · 2021-02-05T19:06:28Z

just a quick note: because this is a release branch (next) which has special meaning to semantic-release, do not do any rebasing and force pushing. And we should merge this PR using Rebase and merge instead of squashing. semantic-release is using git notes to keep track of preview releases. If we would do any squashing or force-pushing we would loose that state

G-Rath · 2021-02-05T19:10:04Z

Yeah, sorry I didn't know it was setup to do a pre-release (which is very cool, and didn't know you could do - might "borrow" that config for some of my other projects) 😓

Any follow up changes I'll do to this branch via PR, but it seems like it's good to go already 🤷

Are there any other breaking changes you'd talked about doing previously that could be included?

(also @gr2m I'll let you do the merging for this one 🙂)

gr2m

we can also remove this from the tests I think?

webhooks.js/test/typescript-validate.ts

Lines 199 to 206 in 2db33e5

    
           // ************************************************************ 
        
           // DEPRECATIONS RETRO-COMPATIBILITY 
        
           // ************************************************************ 
        
           export function webhookErrorTestDeprecated(error: WebhookError) { 
        
             const { request } = error; 
        
             console.log(request); 
        
           }

scripts/generate-types.ts

gr2m · 2021-02-05T19:59:44Z

I wonder, now that we have explicit event handlers .onAny and .onError, we should probably throw if someone tries to listen to "*" or "error" events, as we use these strings internally to keep track of the .onAny/.onError calls. Rather an edge case and it would fail when compiling TypeScript, but might be worth to throw a useful error anyway at least for v8.x to help people upgrade

gr2m · 2021-02-05T20:18:37Z

btw next time let's create a beta branch for pre-releases, that will create 8.0.0-beta.1, 8.0.0-beta.2, etc. Once we think it's ready, we can still go through a next branch to have more people test it before flagging it as @latest on npm/GitHub releases

G-Rath · 2021-02-07T22:22:00Z

Bamn! I've refactored the types based off #441 (thanks @jablko ❤️), and updated the typescript section of the README.

I think this is now good to go, unless anyone has any other changes they think we could ship in this major?

The only other thing off the top of my head that I could do is renaming the types to be consistent - they're mostly already fine now, but we could rename EmitterWebhookEvent to WebhookEvent as its a bit shorter and now makes more sense? Plus a few others 🤔

src/event-handler/on.ts

jablko · 2021-02-08T16:52:14Z

src/types.ts

+} from "@octokit/webhooks-definitions/schema";
+import type { emitterEventNames } from "./generated/webhook-names";
+
+export type EmitterWebhookEventName = typeof emitterEventNames[number];


What made you decide to revert the type alias in the generated type? #441 (comment)

I don't have a strong opinion, just curious.

I just found it was nicer to have everything being imported from the same place, rather than having an extra import line 😅

jablko · 2021-02-08T16:52:21Z

Can we land #443, #447, and #441, which this branch incorporates, to separate the breaking from non-breaking changes?

jablko · 2021-02-08T17:33:28Z

src/types.ts

+
+export type EmitterWebhookEventName = typeof emitterEventNames[number];
+export type EmitterWebhookEvent<
+  TEmitterEvent extends EmitterWebhookEventName = EmitterWebhookEventName


Suggested change

TEmitterEvent extends EmitterWebhookEventName = EmitterWebhookEventName

TEmitterEvent extends EmitterWebhookEventName = WebhookEventName

I don't know if it makes a difference, but in #441 I went with WebhookEventName here because EmitterWebhookEvent<WebhookEventName> is functionally equivalent to EmitterWebhookEvent<EmitterWebhookEventName> and WebhookEventName is smaller than EmitterWebhookEventName so I thought it might perform better? https://github.com/jablko/webhooks.js/compare/patch-2..octokit:next#diff-c54113cf61ec99691748a3890bfbeb00e10efb3f0a76f03a0fd9ec49072e410aL14-R10

I'd prefer to use the most accurate type - if it was causing more performance problems, I'd want to know about them here since you could still run into them in userland by using that type :)

jablko · 2021-02-08T17:45:11Z

src/types.ts

-> {
+import { RequestError } from "@octokit/request-error";
+import type {
+  WebhookEventMap,


Suggested change

WebhookEventMap,

EventPayloadMap,

No strong opinion, but to my eye EventPayloadMap makes it clearer that this is a map from WebhookEventName -> payload vs. WebhookEventName -> event? https://github.com/jablko/webhooks.js/compare/patch-2..octokit:next#diff-c54113cf61ec99691748a3890bfbeb00e10efb3f0a76f03a0fd9ec49072e410aL3-R3

It's a map of webhook events - the fact that it's called "Payload" is a remnant of previous versions, and at some point will likely be removed.

G-Rath · 2021-02-08T17:51:03Z

Can we land #443, #447, and #441, which this branch incorporates, to separate the breaking from non-breaking changes?

I class all these changes as breaking, because of the size of the refactors, and would just in general like to get v8 landed.
While we could land these in 7.x, if there is further bug fixing to be done we'd not be able to land those once we've landed 8.x, so might as well just go to 8.x.

I don't mind reverting the all the type refactor stuff in 7.x so that people can stay on that longer if they want?

@gr2m while everyones approved this, I'll leave the releasing in your hands since I don't want to mess it up 😅

jablko · 2021-02-08T18:14:24Z

src/types.ts

+} from "@octokit/webhooks-definitions/schema";
+import type { emitterEventNames } from "./generated/webhook-names";
+
+export type EmitterWebhookEventName = typeof emitterEventNames[number];


Suggested change

export type EmitterWebhookEventName = typeof emitterEventNames[number];

export type EmitterEventName = typeof emitterEventNames[number];

What do you think of just EmitterEventName, now every EmitterEventName is an EmitterWebhookEventName ("*" and "error" having been removed)?

yup, but I'm not so concerned about that since its still accurate with the current name, and everyones approved this version 🤷🙂

Following the pattern of TEmitterEvent vs. TWebhookEvent:

webhooks.js/src/types.ts

Line 11 in fc5277d

> = TEmitterEvent extends `${infer TWebhookEvent}.${infer TAction}`

gr2m · 2021-02-08T18:37:10Z

Can we land #443, #447, and #441, which this branch incorporates, to separate the breaking from non-breaking changes?

I class all these changes as breaking, because of the size of the refactors, and would just in general like to get v8 landed

I agree. Sorry @jablko. The upgrade to v8 shouldn't be too hard.

@gr2m while everyones approved this, I'll leave the releasing in your hands since I don't want to mess it up 😅

I got it

gr2m · 2021-02-08T18:46:00Z

well the automation didn't work for some reason 🤷🏼 but I've fixed it manually. Version 8.0.2 is now the latest on npm and GitHub

G-Rath · 2021-02-08T18:49:37Z

Feel free to blame that on me 😅 next time I'll not call the branch next

oscard0m · 2021-02-08T22:00:13Z

Besides the automatic release not working, how was it? All working and shinning?

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

🎉 This PR is included in version 8.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

G-Rath added 8 commits February 5, 2021 15:38

test: refactor to not catch expects

f0b3196

feat: remove support for error event

b2d5c70

BREAKING CHANGE: passing `error` as an event name is no longer supported

feat: remove support for * event

4b20ca7

BREAKING CHANGE: passing `*` as an event name is no longer supported

feat: remove deprecated properties from error types

bae6624

feat: remove deprecated payload types

ca8c6f1

BREAKING CHANGE: payload types have been renamed and refactored

feat: refactor types

8e81a5a

feat: make @octokit/webhooks-definitions a dependency

c4ed3d8

fix: typos

2db33e5

G-Rath added the Type: Feature New feature or request label Feb 5, 2021

G-Rath requested review from gr2m, oscard0m and wolfy1339 February 5, 2021 04:22

ghost added this to Features in JS Feb 5, 2021

wolfy1339 previously approved these changes Feb 5, 2021

View reviewed changes

jablko mentioned this pull request Feb 5, 2021

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

Closed

gr2m reviewed Feb 5, 2021

View reviewed changes

scripts/generate-types.ts Show resolved Hide resolved

G-Rath mentioned this pull request Feb 5, 2021

🚧 refactor(types): replace EmitterEventWebhookPayloadMap #441

Closed

test: remove test for deprecated pattern

083e92a

G-Rath dismissed wolfy1339’s stale review via 083e92a February 5, 2021 19:20

G-Rath requested review from wolfy1339 and gr2m February 5, 2021 19:20

gr2m changed the title ~~Next~~ v8.0.0 Feb 5, 2021

fix: throw an error if * or error events are passed to #on

560ff73

G-Rath mentioned this pull request Feb 7, 2021

test(typescript): add case ensuring action differences are typed #442

Merged

refactor(typescript): remove need for webhook payload type map

0069a1e

G-Rath and others added 2 commits February 8, 2021 11:18

docs: update typescript section of readme

cdd27dd

docs: update ToC for README.md

fc5277d

G-Rath marked this pull request as ready for review February 7, 2021 22:37

wolfy1339 reviewed Feb 8, 2021

View reviewed changes

src/event-handler/on.ts Show resolved Hide resolved

This comment has been minimized.

Sign in to view

wolfy1339 previously approved these changes Feb 8, 2021

View reviewed changes

jablko reviewed Feb 8, 2021

View reviewed changes

Merge branch 'master' into next

2ef9532

gr2m dismissed wolfy1339’s stale review via 2ef9532 February 8, 2021 18:35

gr2m merged commit 1891034 into master Feb 8, 2021

JS automation moved this from Features to Done Feb 8, 2021

gr2m deleted the next branch February 8, 2021 18:38

jablko mentioned this pull request Feb 8, 2021

refactor(types): rename EmitterWebhookEventName -> EmitterEventName #452

Closed

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

prince-chrismc mentioned this pull request Mar 21, 2021

Update location of typescript definitions actions/toolkit#743

Merged

wolfy1339 mentioned this pull request Apr 15, 2023

Adapt the types to the OpenAPI spec types #840

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v8.0.0 #444

v8.0.0 #444

G-Rath commented Feb 5, 2021 •

edited by gr2m

G-Rath commented Feb 5, 2021

gr2m commented Feb 5, 2021

G-Rath commented Feb 5, 2021 •

edited

gr2m left a comment

gr2m commented Feb 5, 2021

gr2m commented Feb 5, 2021

G-Rath commented Feb 7, 2021

This comment has been minimized.

jablko Feb 8, 2021

G-Rath Feb 8, 2021

jablko commented Feb 8, 2021 •

edited

jablko Feb 8, 2021

G-Rath Feb 8, 2021

jablko Feb 8, 2021

G-Rath Feb 8, 2021

G-Rath commented Feb 8, 2021

jablko Feb 8, 2021

G-Rath Feb 8, 2021

jablko Feb 8, 2021

gr2m commented Feb 8, 2021

gr2m commented Feb 8, 2021

G-Rath commented Feb 8, 2021

oscard0m commented Feb 8, 2021

github-actions bot commented Feb 11, 2021

	// ************************************************************
	// DEPRECATIONS RETRO-COMPATIBILITY
	// ************************************************************

	export function webhookErrorTestDeprecated(error: WebhookError) {
	const { request } = error;
	console.log(request);
	}

	TEmitterEvent extends EmitterWebhookEventName = EmitterWebhookEventName
	TEmitterEvent extends EmitterWebhookEventName = WebhookEventName

	export type EmitterWebhookEventName = typeof emitterEventNames[number];
	export type EmitterEventName = typeof emitterEventNames[number];

v8.0.0 #444

v8.0.0 #444

Conversation

G-Rath commented Feb 5, 2021 • edited by gr2m

G-Rath commented Feb 5, 2021

gr2m commented Feb 5, 2021

G-Rath commented Feb 5, 2021 • edited

gr2m left a comment

Choose a reason for hiding this comment

gr2m commented Feb 5, 2021

gr2m commented Feb 5, 2021

G-Rath commented Feb 7, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jablko commented Feb 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m commented Feb 8, 2021

gr2m commented Feb 8, 2021

G-Rath commented Feb 8, 2021

oscard0m commented Feb 8, 2021

github-actions bot commented Feb 11, 2021

G-Rath commented Feb 5, 2021 •

edited by gr2m

G-Rath commented Feb 5, 2021 •

edited

jablko commented Feb 8, 2021 •

edited