Enable exactOptionalPropertyTypes option in tsconfig

[oscard0m]

What’s missing?

To enable exactOptionalPropertyTypes option in Octokit's tsconfig

Why?

Promoted by octokit/request-error.js#461, @wolfy1339 and I think it can be a good plan to enable this option.

The benefits are:

• Users of octokit with exactOptionalPropertyTypes: true option won't have problems. Here you can see an example with the option enabled when using @octokit/request-error.js.
• Required, but potentially undefined does not have the same intent as optional. This rule will enforce us to be explicit in our type declarations and how we expect octokit APIs should be used.¹

Migration plan:

Enabling this feature, considering our multirepo architecture requires a progressive migration. The 2 alternatives I come up with are:

Upgrading each package tsconfig individually and finally upgrade @octokit/tsconfig

1. A first idea I come up with is upgrading each package's tsconfig.json individually and adding the corresponding changes to the code, if any. Even this does not imply changes in code, I assume this should be a BREAKING CHANGE because our types become more strict to our users. Is this correct?
2. Once all the packages are upgraded, we can finally update @octokit/tsconfig by adding this new option.
3. We wait for renovate to bump @octokit/tsconfig in all our packages.
4. We re-visit all the packages (maybe with octoherd?) and remove the explicit rule in local's tsconfig.json

Working with beta branches

1. We release a beta version of @octokit/tsconfig with the new property
2. We create beta branches for each of Octokit's packages using this new version, and we apply the corresponding changes to the code if necessary.
3. Once everything is ready, we release a new BREAKING CHANGE for all the packages.

Warning

This change implies a BREAKING CHANGE to all packages. Is this correct?

A good blog explaining its potential benefits it can be found here: https://tkdodo.eu/blog/optional-vs-undefined

Footnotes

1. https://tkdodo.eu/blog/optional-vs-undefined ↩

[oscard0m]

@wolfy1339 I would like to hear your thoughts on the migration plan. What would be the best way to do this? I want to give more priority to user's smoothness. If we need to do more work (like octoherd scripts, I'm fine with it).

Also, I think you have a clear idea on the tree of octokit dependencies we have and what's the best order to start with a migration like this.

[wolfy1339]

I think a breaking change for this package is good.

The whole octokit ecosystem starts at @octokit/types and @octokit/openapi-types.
After that it is @octokit/core dependencies: https://npmgraph.js.org/?q=@octokit/core
Afterwards it is the auth strategy plugins and the various plugins
Then @octokit/rest, and @octokit/app dependency trees
Finally, the octokit package

[wolfy1339]

I don't think it's a BREAKING CHANGE, the types are already inferred as | undefined. This change will simply make it explicit

[oscard0m]

If it's not a breaking change, would it be as easy as creating a breaking change for @octokit/tsconfig and wait for Renovate to do the rest (we don't need to bump @octokit/tsconfig in any specific order)?

[wolfy1339]

We will probably have some changes to do, but leave it up to renovate to start the process for us

[oscard0m]

We will probably have some changes to do, but leave it up to renovate to start the process for us

Great. I will update this issue with subtasks of the corresponding PRs so we keep track of the updates.

[github-actions]

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[oscard0m]

Re-opening to keep track of all the repos migration

[wolfy1339]

I think the option isn't working as intended, our tests aren't catching any errors

[oscard0m]

I think the option isn't working as intended, our tests aren't catching any errors

Any repo in particular you are checking so I can take a look?

[oscard0m]

I think the option isn't working as intended, our tests aren't catching any errors

Any repo in particular you are checking so I can take a look?

I still see you are still merging the bump of the version. Does this mean we are good?

[wolfy1339]

https://github.com/octokit/app.js/actions/runs/11020750264/job/30606242843

Some of them are fine, while others aren't being caught

[wolfy1339]

It seems that some repos don't have a dry-run of the typescript build process which would catch these errors.

It explains why the tests are passing but the release isn't

[oscard0m]

It seems that some repos don't have a dry-run of the typescript build process which would catch these errors.

It explains why the tests are passing but the release isn't

Should we open PRs to dry-run the typescript build process first?

[wolfy1339]

Simply adding npm run build to the test job would work fine.

Let's make some PRs