search

octokit / tsconfig

TypeScript configuration for Octokit packages
MIT License
5 stars 3 forks source link

Enable `exactOptionalPropertyTypes` option in `tsconfig` #29

Open oscard0m opened 1 month ago

oscard0m commented 1 month ago

What’s missing?

To enable exactOptionalPropertyTypes option in Octokit's tsconfig

Why?

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

The benefits are:

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

oscard0m commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

oscard0m commented 1 month ago

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 commented 1 month ago

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

oscard0m commented 1 month ago

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[bot] commented 1 month ago

:tada: This issue has been resolved in version 4.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

oscard0m commented 1 month ago

Re-opening to keep track of all the repos migration

wolfy1339 commented 1 month ago

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

oscard0m commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

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

wolfy1339 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

Let's make some PRs