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.

Sign up for GitHub

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

Jump to bottom

Be careful to avoid breaking changes in with the typescript types. #1439

Closed
JoelEinbinder opened this issue Mar 20, 2020 · 3 comments
Closed

Be careful to avoid breaking changes in with the typescript types. #1439

JoelEinbinder opened this issue Mar 20, 2020 · 3 comments
Assignees
@JoelEinbinder

Comments

@JoelEinbinder
Copy link
Contributor

Right now, its easy to introduce breaking changes with our typescript types without realizing it.

  • Object literal types get names like ClassMethodArgument. If the argument name changes, the type name changes, and that is a breaking change.
  • If we have dog.speak(options) we will have a DogSpeakOptions type. If in a later version, we move speak to the animal class which dogs inherits from, we will only have AnimalSpeakOptions and DogSpeakOptions will be removed. This will be a breaking change.

There are probably some other subtle ways to introduce breaking changes that I have no thought of.

Possible mitigations

  • Add a test that checks that current types are a strict superset of the previously released types, unless this revision is a breaking change.
  • Manually add old types back into the api to make sure there are no breaking changes.
  • Don't export all parameter type objects, instead manually construct a set of useful types to export, and keep that from having breaking changes.
@JoelEinbinder JoelEinbinder mentioned this issue Mar 20, 2020
Merged
@dgozman dgozman added the v1 label Mar 22, 2020
@dgozman
Copy link
Contributor

dgozman commented Mar 30, 2020

  • Don't export all parameter type objects, instead manually construct a set of useful types to export, and keep that from having breaking changes.

I want to try this approach. This way we can be sure our types are good. For example, we currently have PageFilechooserPayload which should actually be an object with methods (to account for feature creep), but currently it ha a bunch of optional properties.

@JoelEinbinder JoelEinbinder mentioned this issue Mar 31, 2020
Merged
JoelEinbinder added a commit that referenced this issue Mar 31, 2020
@JoelEinbinder
fix(types): don't export derived types (#1598)
a853690 
I was playing around today with different ways of changing the way we export types for #1439. I looked at only exporting 'Parameter' types, only exporting 'Return' types, only exporting a manual list of 'important' types. They all had different pros and cons, and it was very difficult to settle on a good answer.

For now, let's not export any parameter/return types. We can whitelist some types upon user request. I'm thinking `LaunchOptions` and `AccessibilitySnapshot` could be quite useful. We can always add new types after 1.0, but we can't remove them.

The patch looks funny because this was my original intent for the types, but I didn't know I had to `export {}` to tell typescript that my .d.ts shouldn't export everything.
@JoelEinbinder
Copy link
Contributor Author

This is mostly done, except that we should add a test that lints our current types against the old ones.

@JoelEinbinder JoelEinbinder removed the v1 label Apr 3, 2020
@JoelEinbinder
Copy link
Contributor Author

I tried to add a test that lints our current types against the old ones. It was stymied in two ways.

  • doc: fix the route docs #2174 appears to have broken the types, but it actually fixed the types where they didn't match the implementation
  • our CDPSession types break pretty much every release, but we don't make any guarantees here.

Closing this as infeasible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JoelEinbinder JoelEinbinder

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@JoelEinbinder @dgozman