web worker: TypeScript and rollup #2289
Conversation
packages/web-worker/src/bugsnag.ts
Outdated
| const name = 'Bugsnag Web Worker' | ||
| const url = 'https://github.com/bugsnag/bugsnag-js' | ||
| // @ts-ignore | ||
| const version = __BUGSNAG_NOTIFIER_VERSION__ // eslint-disable-line no-undef |
There was a problem hiding this comment.
Not sure why but there seems to be a problem with this being __VERSION__ specifically. If I try to use that rollup blows up with:
[!] RollupError: src/bugsnag.ts (14:25): Expected a semicolon (Note that you need plugins to import files that are not JavaScript)
src/bugsnag.ts (14:25)
12: var name = 'Bugsnag Web Worker';
13: var url = 'https://github.com/bugsnag/bugsnag-js';
14: var version = __VERSION__;
^
15: var schema = assign({}, baseConfig, workerConfig);
packages/browser/src/bugsnag.ts
Outdated
| const version = '__VERSION__' | ||
| // @ts-ignore | ||
| const version = __BUGSNAG_NOTIFIER_VERSION__ // eslint-disable-line no-undef |
There was a problem hiding this comment.
fix version in browser notifier. maybe we need a test for this, if possible. See also comment below about strange problems with __VERSION__
| "build:dist:esm": "webpack --config esm.config.js", | ||
| "build:dist:esm.min": "webpack --config esm.config.js --optimization-minimize --output-filename=bugsnag.web-worker.min.mjs", |
There was a problem hiding this comment.
these don't appear to have been used. it looks like we're also publishing this to cdn but then there are no docs for that. so do we need to support cdn for web workers?
There was a problem hiding this comment.
I think that's one for @tomlongridge - as far I understand the CDN was a requirement, but if it's not been documented we will need to remedy that
There was a problem hiding this comment.
It sounds like it, but also opens the question about whether we have tests to cover that scenario. I'd suggest ticketing for future, but keep doing what we're doing now as far as publishing is concerned so we don't stop publishing if anyone is using it?
| @@ -33,7 +33,7 @@ const plugins = [ | |||
| preventAssignment: true, | |||
| values: { | |||
| 'process.env.NODE_ENV': JSON.stringify('production'), | |||
| values: { __VERSION__: packageJson.version }, | |||
| __BUGSNAG_NOTIFIER_VERSION__: JSON.stringify(packageJson.version), | |||
There was a problem hiding this comment.
fix version in browser notifier. maybe we need a test for this, if possible. See also comment below about strange problems with __VERSION__
| // configure a client with user supplied options | ||
| const bugsnag = new ClientWithInternals(opts, schema, internalPlugins, { name, version, url }) | ||
|
|
||
| bugsnag._setDelivery(client => delivery(client, self.fetch)) |
There was a problem hiding this comment.
delivery-fetchis implemented likeconst delivery = (client, fetch = global.fetch) => ({`
In webpack this global.fetch gets converted to a webpack require that somehow resolves this properly for web workers. However in rollup it remains as global.fetch which is undefined. We could perhaps replace the default value with globalThis.fetch but I'd think I'd rather see the default omitted requiring the notifier implementation to explicitly state what fetch to use.
| if (!opts) opts = {} as unknown as Config | ||
|
|
||
| const internalPlugins = [ | ||
| pluginBrowserDevice(navigator, null), |
There was a problem hiding this comment.
As per delivery-fetch comment below maybe these plugins would be better off not assuming what environment they are running in
packages/core/client.d.ts
Outdated
| @@ -82,4 +82,6 @@ export default class ClientWithInternals<T extends Config = Config> extends Clie | |||
| _loadPlugin(plugin: Plugin): void | |||
|
|
|||
| _isBreadcrumbTypeEnabled(type: string): boolean | |||
|
|
|||
| public addOnError(fn: OnErrorCallback, front?: boolean): void; | |||
There was a problem hiding this comment.
the public types do not currently expose the second param
There was a problem hiding this comment.
as this is now in the public types, could we name it something to describe what it does? like moveToFront or something?
| const name = 'Bugsnag Web Worker' | ||
| const url = 'https://github.com/bugsnag/bugsnag-js' | ||
| // @ts-ignore | ||
| const version = __BUGSNAG_NOTIFIER_VERSION__ |
There was a problem hiding this comment.
Not sure why but there seems to be a problem with this being __VERSION__ specifically. If I try to use that rollup blows up with:
[!] RollupError: src/bugsnag.ts (14:25): Expected a semicolon (Note that you need plugins to import files that are not JavaScript)
src/bugsnag.ts (14:25)
12: var name = 'Bugsnag Web Worker';
13: var url = 'https://github.com/bugsnag/bugsnag-js';
14: var version = __VERSION__;
^
15: var schema = assign({}, baseConfig, workerConfig);
packages/core/client.d.ts
Outdated
| @@ -82,4 +82,6 @@ export default class ClientWithInternals<T extends Config = Config> extends Clie | |||
| _loadPlugin(plugin: Plugin): void | |||
|
|
|||
| _isBreadcrumbTypeEnabled(type: string): boolean | |||
|
|
|||
| public addOnError(fn: OnErrorCallback, front?: boolean): void; | |||
There was a problem hiding this comment.
as this is now in the public types, could we name it something to describe what it does? like moveToFront or something?
Goal
Design
Changeset
Testing