feat: Use 4.0.0 #89
feat: Use 4.0.0 #89
Conversation
src/common.ts
Outdated
| @@ -10,7 +10,7 @@ export const IPC_EVENT = 'sentry-electron.event'; | |||
| /** IPC to capture a breadcrumb globally. */ | |||
| export const IPC_CRUMB = 'sentry-electron.breadcrumbs'; | |||
| /** IPC to capture new context (user, tags, extra) globally. */ | |||
src/main/backend.ts
Outdated
| */ | ||
| export async function isAppReady(): Promise<boolean> { | ||
| return app.isReady() | ||
| ? Promise.resolve(true) |
src/main/backend.ts
Outdated
| @@ -65,7 +65,7 @@ function getRendererExtra( | |||
| */ | |||
| export async function isAppReady(): Promise<boolean> { | |||
| return app.isReady() | |||
| ? Promise.resolve(true) | |||
| ? true | |||
There was a problem hiding this comment.
nit: app.isReady() || new Promise(...);
src/main/backend.ts
Outdated
| // tslint:enable:no-unsafe-any | ||
| const currentCloned = Scope.clone(getDefaultHub().getScope()); | ||
| const fetchedScope = this.scopeStore.get(); | ||
| (fetchedScope as any).eventProcessors = []; |
There was a problem hiding this comment.
This looks a bit fishy. The stored scope should never have eventProcessors. It might be more defensive to remove the eventProcessors when setting into the store instead of when fetching it...
src/main/normalize.ts
Outdated
| // The culprit has been deprecated about two years ago and can safely be | ||
| // removed. Remove this line, once this has been resolved in Raven. | ||
| delete (event as { culprit: string }).culprit; | ||
| delete (copy as { culprit: string }).culprit; |
There was a problem hiding this comment.
This should not be necessary anymore with the new Node SDK?
src/main/backend.ts
Outdated
| public constructor(frontend: Frontend<ElectronOptions>) { | ||
| this.frontend = frontend; | ||
| this.inner = new NodeBackend(frontend); | ||
| public constructor(private readonly options: ElectronOptions) { |
There was a problem hiding this comment.
nit: This is the only time we declare fields like this.
| ): Promise<void> { | ||
| event.tags = { event_type: 'native', ...event.tags }; | ||
| await this.processEvent( | ||
| event, |
There was a problem hiding this comment.
nit: In future refactoring, consider: { ...event, tags: { event_type: 'native', ...event.tags } }
|
I'll take a look and test this with our app on Monday |
| const nodeClient = getDefaultHub().getClient() as NodeClient; | ||
| await nodeClient.captureException(error, getDefaultHub().getScope()); | ||
|
|
||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
I'm not sure about this logic. When an app is packaged and run on a users machine, NODE_ENV will usually be undefined.
In the main process, you can detect if the app is packaged via:
process.defaultApp === undefined
Many will also want a way to not display the dialog in development as its
a) too narrow to properly display stack traces
b) you can't copy and paste from the dialog
| export function specificInit(options: ElectronOptions): void { | ||
| process.type === 'browser' | ||
| ? module.require('./main').init(options) | ||
| : require('./renderer').init(options); |
There was a problem hiding this comment.
It's confusing I know https://electronjs.org/docs/api/process#processtype
This PR uses the
4.0.0-beta.xpackages internally now which mostly causes internal refactorings/rewrites.There is a breaking change with context/scope handling which is documented in the changelog / docs.
On
uncaughtExceptionif not inprodwe show the dialog the same way as electron does, if inprodwe exit the app if the user does not overwrite this behavior withonFatalErroroption.