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

Moving index.json to a different package #362

Closed
oscard0m opened this issue Feb 7, 2021 · 14 comments · Fixed by #411 or #436
Closed

Moving index.json to a different package #362

oscard0m opened this issue Feb 7, 2021 · 14 comments · Fixed by #411 or #436
Labels
Type: Feature New feature or request
Projects

Comments

@oscard0m
Copy link
Member

oscard0m commented Feb 7, 2021

What’s missing?
Would be nice to not pack examples when publishing this package.

Why?
Avoid extra package size

Context: octokit/webhooks.js#439 (comment)

@oscard0m oscard0m added the Type: Feature New feature or request label Feb 7, 2021
@ghost ghost added this to Features in JS Feb 7, 2021
@oscard0m
Copy link
Member Author

oscard0m commented Feb 7, 2021

A scenario coming to my mind which could be worrying is probot used in lambda functions (where the size very limited for serverless)

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

You beat me to it 😅 I'll paste my issue body here :)

@wolfy1339
Copy link
Member

Perhaps the package name of @octokit/webhooks-examples would be good for this?

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

The payloads are big, and make up the majority of the published 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

They were originally being shipped to power type generation, which is now obsolete as we're shipping JSON schemas and generating more accurate types from those.

Since these types are being used in types that are published in downstream packages, we're wanting to include this package as a production dependenecy meaning all that size will be pulled into the dependency tree when all that's really needed is the far smaller schema.d.ts.

Additionally, the payloads are currently being shipped as the "main" entity for this repo, meaning doing require('@octokit/webhooks-definitions gives you the payload examples rather than main schema (and likewise for the types), which I think is counter interactive given the package name.

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

A scenario coming to my mind which could be worrying is probot used in lambda functions (where the size very limited for serverless)

For sure - serverless in general; i.e compare-gh-webhook-to-schema will have the whole package in production right now (I've not yet configured the exclude to strip it out)

@wolfy1339
Copy link
Member

For whenever we get around to doing this, we can spin out the payload examples directory into another repo while saving their git history using this technique

@gr2m
Copy link
Contributor

gr2m commented Feb 7, 2021

For context: https://github.com/octokit/webhooks/ was not meant to be a production dependency, similar to https://github.com/octokit/openapi. Moving the examples out of the repository will make it harder to contribute. I really like the fact that anyone can just submit a payload example as a means to reproduce a problem with the current schemas.

Also keep in mind that this repository is meant to be useful across all language ecosystems, not just for JavaScript. There are other SDKs besides the JS ones. They don't use this repository yet, but the goal for this repository is to be a shared resource in future.

And if we are considering to move out the examples, why not consider to move out the types instead, similar to https://github.com/octokit/openapi-types.ts?

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

Moving the examples out of the repository will make it harder to contribute.

I think there are two things here: publishing them in this package, and publishing them in a package - I actually think we should focus on the latter before the former, since as you've said there is a very nice flow in having the payloads in this repo which I think we could only keep if we moved to a mono-repo if we want to publish a separate package.

(so, imo it's a lot easier for us to discuss & action the latter now than the former)


why not consider to move out the types instead

In my case, this won't solve the problem as I'm using the schemas not the types :)

@gr2m
Copy link
Contributor

gr2m commented Feb 7, 2021

I'm not a fan of mono-repos for many reasons, but in this case we could publish dependency-free packages with just the types and one with just the schema which should be much easier ... the packages we would publish would only differ in

  1. package name
  2. published files

The release version, release notes, etc, could be exactly the same?

@G-Rath
Copy link
Member

G-Rath commented Feb 7, 2021

Sure - I think that could go wrong in a few ways, but overall the contents of this repo mean if it did the actual end result wouldn't be horrible.

I'll point out that it feels to me like a mono-repo without the tooling, but I'm neutral on monorepos so I'm happy to go with what you'd prefer :)

My main question that that comment stems from is how do we handle versioning? I think for semantic-release's sanity it'll probably be best if we bump the versions of all three packages for any change, rather than try and juggle them. (you might know a trick or two that can make this work better)

This actually could work nicely and without being breaking, as we can continue to publish @octokit/webhooks-definitions as simply depending on all three packages and re-exporting them.

I propose these packages:

  • @octokit/webhook-types
  • @octokit/webhook-schemas
  • @octokit/webhook-examples

@gr2m
Copy link
Contributor

gr2m commented Feb 7, 2021

I think for semantic-release's sanity it'll probably be best if we bump the versions of all three packages for any change, rather than try and juggle them

yes, definitely.

This actually could work nicely and without being breaking, as we can continue to publish @octokit/webhooks-definitions as simply depending on all three packages and re-exporting them.

sounds good to me.

I just remembered another difference. A package should have a "main" export, even if we just publish typescript, in order to avoid what happened here: gr2m/before-after-hook#87

@oscard0m
Copy link
Member Author

oscard0m commented Feb 7, 2021

My only concern of this mono-repo proposal is what @gr2m pointed before regarding Octokit ecosystem inconsistency. Octokit aims to a multi-repo solution and in this case we are merging approaches in my opinion.

Besides this point, and considering the problem we are facing here, 👍🏽 I'm okay with this proposal:

@octokit/webhook-types
@octokit/webhook-schemas
@octokit/webhook-examples


I think for semantic-release's sanity it'll probably be best if we bump the versions of all three packages for any change, rather than try and juggle them

👍🏽 Agree on this.

@G-Rath
Copy link
Member

G-Rath commented Feb 8, 2021

in order to avoid what happened here

I'm not sure how that's related to a package that is publishing and being used purely for typescript (since the package in question is a js library), but I have no mind in publishing an empty file :)

My preference would be to follow whatever DefinitelyTyped is doing, since they'll be in the same boat for this sort of thing :)

@wolfy1339
Copy link
Member

My preference would be to follow whatever DefinitelyTyped is doing, since they'll be in the same boat for this sort of thing :)

If we take example from @types/react (or any other for that matter), the main field is set to "" and the types field is set to index.d.ts

@wolfy1339 wolfy1339 linked a pull request Mar 21, 2021 that will close this issue
JS automation moved this from Features to Done Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
No open projects
JS
  
Done
4 participants