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

fix(deps): move @octokit/webhooks-definitions to non-dev deps #439

Closed
wants to merge 1 commit into from

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Feb 4, 2021

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented, or is being fixed label Feb 4, 2021
@wolfy1339 wolfy1339 requested review from gr2m and G-Rath February 4, 2021 18:32
@gr2m
Copy link
Contributor

gr2m commented Feb 4, 2021

I'm okay with moving ahead to help resolve #436, but I'm concerned that making @octokit/webhooks-definitions a production dependency might blow up the bundle size when using @octokit/webhooks. If it's only the type we want, maybe we should move them to its own repository, or make it part of @octokit/types? Any thoughts?

@@ -22,13 +22,13 @@
"prettier": {},
"dependencies": {
"@octokit/request-error": "^2.0.2",
"@octokit/webhooks-definitions": "3.57.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a tough one - I think if we're going to make this a dependency (peer or otherwise) we should loosen our constraints to enable deduplication and getting fixes etc.

This will technically open us up for becoming out of sync, but the chances and implications of that are very minor: the only changes in the library that impacts us here is if we add/remove/change an event (as in, event name), which can be worked around as outlined in #434 (i.e, with casting, and because we're not erroring on unknown events, the user would just get a warning).

@G-Rath
Copy link
Member

G-Rath commented Feb 4, 2021

@gr2m you beat me to it 😅

While I think in the long-run moving them to @octokit/types is probably a good idea, I think we don't have to have this as a dependency for this library - this is how it's commonly been for TypeScript dependencies for a while, and theres no perfect solution.

So either we:
A. include the dependency as a production dependency, which'll increase the bundle size but allow TS users to not know about this dependency
B. document that you need to install this extra package if you're using TS, and keep this as a dev dependency.

I think B. is probably the way to go given the size of things, and the way bundles work (specifically, they'll often just always include it in full as its a production dependenecy, with little way to exclude)

@gr2m
Copy link
Contributor

gr2m commented Feb 4, 2021

I'm trying to avoid B if possible, I did so far with all the other @octokit/* packages.

I think I'd go with A for now. I think the event types should go in a separate repository or into @octokit/types eventually, and eventually we will have to figure out a way for users to extend the existing event types in order to support custom events. Custom events are not only for users who want to use @octokit/webhooks for their own made-up events, but also for GitHub and its partners who want to test new webhook events that are still in development.

@G-Rath G-Rath mentioned this pull request Feb 5, 2021
@oscard0m
Copy link
Member

oscard0m commented Feb 7, 2021

Is it possible to get a bundle size comparison before and after this change?

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

@oscard0m that's very dependent on how you're bundling - since we never import the dependency in js code (only in TS) it should be very easy for bundlers to not include it (i.e webpack & parcel should out of the box).

The only reason it'd get included is if the bundler just rolled in every production-level dependency, which'll bring in a lot of other things too. This change will mean packers like serverless will include the package (as they strip out devDependencies manually since they don't do any actual code bundling) but that can be easily accounted for by configuring a manual exclude.

(I'm interested in the size changes too, just wanted to give some background info to showcase that "bundle size" is a very contextual thing)

@wolfy1339
Copy link
Member Author

Is it possible to get a bundle size comparison before and after this change?

@octokit/webhooks-definitions is currently 3.71MB
@octokit/webhooks is currently 294 kB

That makes a total of 4MB
(data taken from npmjs.com)

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

btw, I think it's worth considering how we could ship the payload examples in their own package too at some point, as they're completely unneeded for the schema validation (tbh idk what use-case they have now in the published package that we've got the schemas).

@gr2m
Copy link
Contributor

gr2m commented Feb 7, 2021

we could ship the payload examples in their own package

I agree, now that we have the schema, I think the examples are no longer necessary. I don't mind publishing them though, this will become less of an issue once we stop having @octokit/webhooks-definitions as production dependency and instead find an alternative way to get the TypeScript definitions

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

this will become less of an issue once we stop having @octokit/webhooks-definitions as production dependency and instead find an alternative way to get the TypeScript definitions

I mean, the payloads make up the bulk of the package:

webhooks on  master is 📦 v0.0.0-development took 5s
❯ du -hc schema.json schema.d.ts index.json index.d.ts
428K    schema.json
136K    schema.d.ts
3.0M    index.json
0       index.d.ts
3.6M    total

Removing them would make it far easier to justify having @octokit/webhooks-definitions as a production dependency - it'd still be nice in the long-run, but I think it would take the pressure off us short-term.

@gr2m
Copy link
Contributor

gr2m commented Feb 7, 2021

let's discuss this in https://github.com/octokit/webhooks. I'd say the 4MB extra in the install dependency tree is a problem when it's a problem

@oscard0m
Copy link
Member

oscard0m commented Feb 7, 2021

btw, I think it's worth considering how we could ship the payload examples in their own package too at some point, as they're completely unneeded for the schema validation (tbh idk what use-case they have now in the published package that we've got the schemas).

A use case coming to my mind would be a unit test where a user wants to an example as a mocked response. Still, this would have sense to be a devDependency in its own package

@rarkins
Copy link

rarkins commented Feb 8, 2021

@gr2m FYI this seems to even be blowing up "lock file maintenance" PRs, e.g. https://github.com/renovatebot/auto-cancel-actions/pull/59/checks?check_run_id=1851162719

@gr2m
Copy link
Contributor

gr2m commented Feb 8, 2021

@gr2m FYI this seems to even be blowing up "lock file maintenance" PRs, e.g. https://github.com/renovatebot/auto-cancel-actions/pull/59/checks?check_run_id=1851162719

Yes, I was able to reproduce it.

@wolfy1339 @G-Rath @oscard0m unless any of you object, let's get this merged an released. We can worry about bundle size later (via octokit/webhooks#362)

@gr2m gr2m closed this in #444 Feb 8, 2021
@rarkins
Copy link

rarkins commented Feb 8, 2021

@gr2m does solving this in a major release mean it won't "unbreak" semver-compatible ranges and therefore manual upgrading will be required? I didn't fully understand the interleaving requirements that lead to the breakage so may be mistaken.

@gr2m
Copy link
Contributor

gr2m commented Feb 8, 2021

@gr2m does solving this in a major release mean it won't "unbreak" semver-compatible ranges and therefore manual upgrading will be required

Yes. We could still fix the install problem by creating a pull request against the release-7.x branch. But the fix might introduce other problems and it doesn't seem to be a problem for most of the users. I'd recommend to upgrade.

@gr2m gr2m deleted the fix-octokit-definitions branch February 8, 2021 20:26
@rarkins
Copy link

rarkins commented Feb 8, 2021

Breaking something in a minor release and fixing it in a major does not sound like being in the spirit of semver (again, assuming I'm not misunderstanding what happened here, and I'm still fuzzy).

The thing is I don't actually know what I need to upgrade, as I pin my direct dependencies.

My best guess is that our bot was broken due to a backwards incompatible change in a transitive dependency of probot. If so, can you officially mark all permanently broken versions of probot as deprecated on npmjs so it's more clear?

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 8.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rarkins
Copy link

rarkins commented Feb 12, 2021

^bumping the above unanswered question

@wolfy1339
Copy link
Member Author

Breaking something in a minor release and fixing it in a major does not sound like being in the spirit of semver (again, assuming I'm not misunderstanding what happened here, and I'm still fuzzy).

I fully understand your frustrations. We don't usually consider TypeScript changes as breaking, since it's only used build time.
Feel free to make your point for us to revisit this.

The thing is I don't actually know what I need to upgrade, as I pin my direct dependencies.

You can update to the latest version of probot and @octokit/webhooks is pinned to the last working release before we shipped the new types.

My best guess is that our bot was broken due to a backwards incompatible change in a transitive dependency of probot.

Yes, exactly. It goes probot -> @octokit/webhooks -> @octokit/webhooks-definitions

Whenever there is a breaking change somewhere in the chain, it trickles up to probot

If so, can you officially mark all permanently broken versions of probot as deprecated on npmjs so it's more clear?

That would be up to @gr2m to do.

@gr2m
Copy link
Contributor

gr2m commented Feb 12, 2021

I don't think that any probot version is permanently broken for all users.

I know how semantic versioning works.

We are doing our best to adhere to it. In this case something broke that is not easily reversible because it was a substantial change. Our tests should have caught that but didn't, that shouldn't happen again.

Sorry for the friction @rarkins, but we have to move on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented, or is being fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants