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

switch to generated type declarations #2327

Open
wants to merge 9 commits into
base: integration/typescript
Choose a base branch
from

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Feb 14, 2025

Replace the manually created type definitions with those now being generated by TypeScript.

This exposed a few areas since the previous PR where the types can be improved. However, there is also liberal usage of @ts-expect-error for areas where the typings fail to work whilst avoiding large rewriting of the code.

Copy link

github-actions bot commented Feb 14, 2025

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 55.62 kB 16.68 kB
After 56.67 kB 17.23 kB
± ⚠️ +1,047 bytes ⚠️ +554 bytes

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 2d00ced

Comment on lines -20 to -25
interface LoggerConfig {
debug: (msg: any) => void
info: (msg: any) => void
warn: (msg: any) => void
error: (msg: any, err?: unknown) => void
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One downside to having babel transpile from TypeScript is that it's not type aware. So if you do:

export { LoggerConfig } from './common'

It fails because LoggerConfig is only a type.

Due to our requirement to support lower TS versions we couldn't do export { type LoggerConfig } from './common' which does fix the issue.

So I moved all the types to common.js which are exported like `export * from './common'`` and there are no complaints.

However, the generated types that TyeScript is now producing still do include the type modifier:

export { default } from './bugsnag';
export type { BrowserBugsnagStatic, BrowserConfig } from './bugsnag';
export * from '@bugsnag/core';

And this can't be solved by us internally not using the type modifier.

To get around this we'll have to use https://www.npmjs.com/package/downlevel-dts as a post-processing step on the types.

An added benefit of this is that internally we'll be able to use type modifier and whatever else we like that is supported by our more recent version of TypeScript

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, we can use the type modifier, but not inline.

export type { LoggerConfig } from './common' works with TS 3.8, but not export { type LoggerConfig } from './common'

Base automatically changed from plat-12947-7 to integration/typescript February 17, 2025 13:44
@djskinner djskinner marked this pull request as ready for review February 17, 2025 15:45
@djskinner djskinner requested a review from gingerbenw February 17, 2025 15:45
export default class Client<T extends Config = Config> {
private readonly _notifier?: Notifier
public readonly _notifier?: Notifier
// This ought to be Required<T> but the current version of TypeScript doesn't seem to like it
Copy link
Member

Choose a reason for hiding this comment

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

comment can go now

Comment on lines +129 to +131
id?: string | null | undefined
email?: string | null | undefined
name?: string | null | undefined
Copy link
Member

Choose a reason for hiding this comment

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

doesn't the ? cover undefined already?

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.

2 participants