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

V7: Improve type definitions #682

Merged
merged 4 commits into from Dec 23, 2019
Merged

V7: Improve type definitions #682

merged 4 commits into from Dec 23, 2019

Conversation

bengourley
Copy link
Contributor

The PR improves the types in various ways

  • does away with the hacky way config was extended for each platform
  • makes request, app and device more specific, and reuses their definitions
  • adds more rigourous tests, which are written in typescript – ensuring the types actually match the JS object at runtime

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Dec 20, 2019

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 40.63 kB 12.29 kB
After 40.63 kB 12.29 kB
± No change No change

Generated by 🚫 dangerJS against 7d7b3cb

Copy link
Contributor

@djskinner djskinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for introducing nesting types under AbstractTypes / common?

As a TS user if I want to look at the type signatures of the SDK I think I'd rather see the more concise:

export function notify(error: NotifiableError, onError?: OnErrorCallback): void

compared with:

export function notify(error: common.NotifiableError, onError?: common.OnErrorCallback): void
or
export function notify(error: AbstractTypes.NotifiableError, onError?: AbstractTypes.OnErrorCallback): void

I'm not sure AbstractTypes or common adds much to the signature.

packages/node/types/bugsnag.d.ts Outdated Show resolved Hide resolved
packages/core/types/event.d.ts Outdated Show resolved Hide resolved
@bengourley
Copy link
Contributor Author

I was on the fence about the nesting anyway – I saw it as a good way to structure the miscellanaeous types that were not typically needed by the user but were exported just case. But I have just flattened and exported them all at the top level, which removes the nesting and the dubious name.

@bengourley bengourley merged commit 2792564 into v7 Dec 23, 2019
@bengourley bengourley deleted the v7-types-refactor branch December 23, 2019 11:41
@bengourley bengourley mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants