convert Client, Session, Breadcrumb, Event, Config to TypeScript #2323
Conversation
| @@ -58,6 +58,15 @@ const c = tseslint.config( | |||
| { | |||
| files: ['**/*.json', '**/*.[tj]s?(x)', '**/*.cjs', '**/*.mjs'], | |||
| }, | |||
| // Node environment | |||
| { | |||
| files: ['jest/**/*.[js|mjs]', '**/*/babel.config.js', '**/*/rollup.config.mjs', '.rollup/index.mjs'], | |||
There was a problem hiding this comment.
Not sure the best way to target these. I think maybe we could assume all [js|mjs] are node environment then override the globals later to remove node for certain browser-only packages etc.
| @@ -116,7 +116,9 @@ const notifier: BrowserClient = { | |||
|
|
|||
| type Method = keyof typeof Client.prototype | |||
|
|
|||
| map(['resetEventCount'].concat(keys(Client.prototype)) as Method[], (m) => { | |||
| const clientMethods = Object.getOwnPropertyNames(Client.prototype).concat(['resetEventCount']) as Method[] | |||
There was a problem hiding this comment.
Well something has changed meaning that the class properties are now non-enumerable so keys(Client.prototype) (or Object.keys) no longer works.
| @@ -1,3 +1,6 @@ | |||
| const babelConfig = require('../../babel.config.js') | |||
There was a problem hiding this comment.
Right now for most packages we have TypeScript doing the compilation. However, this seemed to create an issue because parts of core use the spread operator, and TypeScript was complaining about a lack of tslib. After installing tslib then the whole of it appears in the bundle but also being imported at an absolute path unique to my machine.
What I've done here and I think we could do for other packages is to have babel do all the transpiling, leaving TypeScript to generate the type declarations only.
This could be handy as babel is more more granular in what transforms are applied and I think it will give us more control over the bundle size. With TypeScript its "target es5" and no further configuration. This approach is also mentioned in the docs at https://www.npmjs.com/package/@rollup/plugin-typescript.
noForceEmit can be very useful if you use with @rollup/plugin-babel and @babel/preset-typescript. Having @rollup/plugin-typescript only do typechecking / declarations with "emitDeclarationOnly": true while deferring to @rollup/plugin-babel for transpilation can dramatically reduce rollup build times for large projects.
By more granular I mean we can look at using @babel/preset-env and browserslist to make it easier to maintain support levels whilst also having the option of adding/removing additional plugins for whatever other transforms we want e.g. for controlling bundle size
As a side note I have noted the increase in bundle size in integration/typescript but I'm confident we can get it back closer to where it was
| ] | ||
|
|
||
| const external = [/node_modules/] | ||
|
|
||
| export default [ | ||
| createRollupConfig({ | ||
| { |
There was a problem hiding this comment.
createRollupConfig is becoming a problem now. Basically we're overriding everything anyway, except you can't override the typescript plugin it adds
There was a problem hiding this comment.
I think it was basically used to create config for anything except the core package in the performance repo. if it's not working don't feel like we have to make it work.
| // This ought to be Required<T> but the current version of TypeScript doesn't seem to like it | ||
| public readonly _config: Required<Config> |
There was a problem hiding this comment.
I did mange to work this out in the end and sorted it in a subsequent PR:
interface Config {
someVal?: string
}
class Client<T extends Config> {
private readonly _config: T & Required<Config>;
constructor(config: T) {
this._config = { ...config, someVal: 'hello' };
}
public helloWorld() {
this._config.someVal.length
}
}
| @@ -270,6 +270,7 @@ describe('Client', () => { | |||
| }) | |||
| client._setDelivery(client => ({ | |||
| sendEvent: (payload) => { | |||
| // @ts-expect-error Property 'errorMessage' does not exist on type 'Error' | |||
There was a problem hiding this comment.
These are fixed in the next PR by introducing a BugsnagError interface
| @@ -21,6 +21,7 @@ describe('Event', () => { | |||
| { foo: 10 }, | |||
| { toJSON: () => { throw new Error('do not serialise me, srsly') } } | |||
| ]) | |||
| // @ts-expect-error Property 'stacktrace' does not exist on type 'Error' | |||
There was a problem hiding this comment.
Fixed in the next PR by introducing a BugsnagError interface
| this._user = { id, email, name } | ||
| } | ||
|
|
||
| toJSON () { | ||
| return { | ||
| payloadVersion: '4', | ||
| // @ts-expect-error - TS doesn't know that the error object has an errorMessage property |
There was a problem hiding this comment.
Fixed in the next PR by introducing a BugsnagError interface
| @@ -6,10 +6,8 @@ module.exports = api => { | |||
| const overrides = [] | |||
|
|
|||
| if (api && api.env('test')) { | |||
| presets.push(['@babel/preset-env', {targets: {node: 'current'}}]) | |||
There was a problem hiding this comment.
in jest we only need to apply the transforms that are needed for whatever node environment we're running
| private readonly Client: typeof Client | ||
| private readonly Event: typeof Event | ||
| private readonly Breadcrumb: typeof Breadcrumb | ||
| private readonly Session: typeof Session |
There was a problem hiding this comment.
I've generally started out with things being private readonly and loosened as required. In the next PR where the generated type defs are used instead of the manually created ones even more of this properties become public
convert Client, Session, Breadcrumb, Event, Config to TypeScript
I've attempted to minimise changes to the actual source code hence the proliferation of
@ts-ignoredirectives. The reality is that some approaches that have been used when written in JavaScript (example the session delegate) aren't best suited to static typing.