fix: pin to a specific version of @octokit/types

[timrogers]

When the @octokit/types package is updated, we run the "update" workflow in GitHub Actions to update this code and push a PR.

At the moment, we're seeing fairly regular bugs affecting this plugin (e.g. octokit/types.ts#426) where it is used with an incompatible version of @octokit/types, so new types that have been introduced are not there.

The reality is that this plugin is only guaranteed to work with a specific version of the types. If you end up updating this plugin but not the types (which is what npm will do naturally), they can be out of sync.

This doesn't only apply to what might seem like major version "breaking changes" to the types. If the types introduce a new endpoint, and you haven't upgraded, then this package will be broken because it will refer to that endpoint without the types being present.

This tweaks the "update" task so it sets in the package.json an exact version of @octokit/types which should be used. This ensures that it is used with a compatible version that does not generate errors.

[timrogers]

I'm very open to feedback on this PR. I could definitely be missing something, but it looks like it will fix some undesirable behaviour that is causing issues for users.

[wolfy1339]

You'll need to update the package.json as well

[timrogers]

I believe that this should update the package.json when it is run - at least from my experience. Do you mean that you think I should commit that change?

[wolfy1339]

Do you mean that you think I should commit that change?

Yes, I think you should commit the change

[timrogers]

@wolfy1339 Done, and ready for re-review ✨

[github-actions]

🎉 This PR is included in version 6.1.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[gr2m]

This is not a good idea. We have @octokit/types as dependency in multiple places. If we are not careful, we will end up with multiple versions of @octokit/types in the same project and that will result in very hard to debug problems and much bigger dependency tree. Installing with @octokit/types@latest already sets the minimum required version, it should be good enough. There were some problems in the past few weeks because properties have been removed, but that doesn't occur very often in the REST API. We had more of these because of the catch up since last October

[timrogers]

This is not a good idea. We have @octokit/types as dependency in multiple places. If we are not careful, we will end up with multiple versions of @octokit/types in the same project and that will result in very hard to debug problems and much bigger dependency tree.

Interesting! Do you think we should revert this, in that case? Perhaps I was a little too hasty to merge this, but it's not too late to change our minds.

Should we instead have a special step of updating the package.json dependency if and only if fields have been removed?

I didn't know that it was possible to end up with multiple versions in the same project - how does that happen?

Installing with @octokit/types@latest already sets the minimum required version, it should be good enough. There were some problems in the past few weeks because properties have been removed, but that doesn't occur very often in the REST API. We had more of these because of the catch up since last October

Is it definitely true that "Installing with @octokit/types@latest already sets the minimum required version"?

If that is the case, then I don't understand how issues like octokit/types.ts#426 can happen. It seems like npm was sticking with an older version, and the user had to manually update the types.ts version to escape their issue. But perhaps I am totally misunderstanding!

[wolfy1339]

This is a quick list of packages using @octokit/types as a dependency

• @octokit/plugin-paginate-rest
• @octokit/plugin-rest-endpoint-methods
• @octokit/auth-callback
• @octokit/plugin-enterprise-server
• @octokit/auth-token
• @octokit/auth-app
• @octokit/graphql
• @octokit/auth-oauth-user
• @octokit/auth-unauthenticated
• @octokit/core
• @octokit/auth-oauth-device
• @octokit/request-error
• @octokit/plugin-enterprise-cloud
• @octokit/endpoint
• @octokit/request
• @octokit/plugin-create-or-update-text-file
• @octokit/plugin-enterprise-compatibility
• @octokit/auth-oauth-app
• @octokit/plugin-retry
• @octokit/plugin-throttling
• @octokit/action
• @octokit/app
• octokit
• @octokit/oauth-methods

If you have a combination of these in your dependencies, of which you already have if you use @octokit/rest, @octokit/graphql, @octokit/request and octokit

The problem of multiple @octokit/types versions would arise if one dependency updates but another hasn't, and the version of @octokit/types they depend on is different

[timrogers]

Thanks for explaining! I assumed npm would use “strictest wins” in the case where multiple packages have the same dependency with different versions, but it sounds like it allows multiple versions instead.

That seems like a good reason not to do what I’ve done here! I am wondering if there is a solution to the problem though.

[oscard0m]

This is an interesting discussion. Would octokit-next.js monorepo approach help to be aligned with all dependencies, or still the same problem?

---

While we don't reach that point:

Adding octokit-types as peerDependency would help here? I think we are following this approach for @octokit/core.

Considering npm >= 7 installs peerDependencies and not all the users want to be blocked by a TypeScript dependency, we can mark this dependency as optional so at least we give feedback to the users.

[timrogers]

I have now reverted this, but I still think we should continue this conversation - as much as I can't contribute much as a JavaScript noob!