Add to docs how to init sentry-electron when using a bundled electron ESM main thread

tom2strobl commented 1 day ago

Is there an existing issue for this?

[x] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[x] I have reviewed the documentation https://docs.sentry.io/
[x] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Electron SDK Version

5.6.0

Electron Version

32.1.2

What platform are you using?

MacOS

Link to Sentry event

No response

Steps to Reproduce

Preface: to my knowledge one should not use top level await to avoid electron deadlocks with the ready event

I'm aware that sentry needs load and initialize before electron, and I saw the recommendation to await generously so my first intention was:

main.ts that gets electron cli'ed (minimal reproduction)

async function initializeSentry() {
  const Sentry = await import('@sentry/electron/main')
  Sentry.init({ dsn: process.env.SENTRY_DSN })
}

async function bootApp() {
  await initializeSentry()
  const { app } = await import('electron')
  await app.whenReady()
  console.log('we are ready')
}

bootApp()

results in SentryError: Sentry SDK should be initialized before the Electron app 'ready' event is fired

So I tried to move up everything:

import * as Sentry from '@sentry/electron/main'
Sentry.init({ dsn: process.env.SENTRY_DSN })

async function bootApp() {
  await initializeSentry()
  const { app } = await import('electron')
  await app.whenReady()
  console.log('we are ready')
}

bootApp()

which now results in some cryptic hacky opentelementry require business

TypeError: Cannot read properties of undefined (reading 'require')
    at new Hook (file:///<somepath>/main.bundle.dev.js:37874:40)
    at __webpack_modules__.../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/RequireInTheMiddleSingleton.js.RequireInTheMiddleSingleton._initialize (file:///<somepath>/main.bundle.dev.js:21426:9)
    at new RequireInTheMiddleSingleton (file:///<somepath>/main.bundle.dev.js:21422:14)
    at __webpack_modules__.../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/RequireInTheMiddleSingleton.js.RequireInTheMiddleSingleton.getInstance (file:///<somepath>/main.bundle.dev.js:21479:68)
    at new InstrumentationBase (file:///<somepath>/main.bundle.dev.js:21584:132)
    at new UndiciInstrumentation (file:///<myname>/dev/fugoya/desktop-release/app/dist-dev/main.bundle.dev.js:20399:9)
    at Object.setupOnce (file:///<somepath>/main.bundle.dev.js:57026:31)
    at setupIntegration (file:///<somepath>/main.bundle.dev.js:43076:17)
    at file:///<somepath>/main.bundle.dev.js:43047:7
    at Array.forEach (<anonymous>)

Note that as you should for cutting startup time I'm bundling the main thread using webpack. I'm bundling for true ESM output without createRequire and dynamic imports:

const webpackConfig = {
  // what we have to do is to essentially tell Webpack to actually use ESM format as well as standard ESM loading mechanism. We need to change our target from node20 to esX like ES2020, ES2022, etc. But changing the target to something other than node means we have to manually feed Webpack information about what built-in modules should be excluded as those can be assumed to be provided by runtime.
  target: 'es2022',

  externals: electronMainExternals,

  entry: {
    pdfWorker: [join(mainDir, 'domains', 'templates', 'pdfWorker.ts')],
    main: join(mainDir, 'main.ts')
  },

  output: {
    library: {
      type: 'module' // used to be commonjs
    },
    chunkFormat: 'module',
    module: true
  },

  module: {
    rules: [
      {
        test: /\.[jt]sx?$/,
        exclude: /node_modules/,
        use: {
          loader: 'swc-loader',
          options: {
            module: {
              type: 'es6'
            },
            jsc: {
              target: 'es2022',
              parser: {
                syntax: 'typescript',
                dynamicImport: true
              }
            }
          }
        }
      }
    ]
  },

  experiments: {
    // enabled top level wait for our generated icon/language files
    topLevelAwait: true,
    // to enable esm output
    outputModule: true
  },

  resolve: {
    extensionAlias: {
      // necessary for typescript / esm since we resolve .ts files from .js extensions in imports
      '.js': ['.ts', '.tsx', '.js', '.jsx'],
      '.jsx': ['.tsx', '.jsx'],
      '.mjs': ['.mts', '.mjs'],
      '.cjs': ['.cts', '.cjs']
    },
    // for resolving esm pkg json export fields
    conditionNames: ['import', 'node']
  },

  // necessary for sentry https://github.com/getsentry/sentry-javascript/issues/12077
  ignoreWarnings: [{ module: /@opentelemetry\/instrumentation/ }],

  plugins: [
    // to upload sourcemaps to sentry
    sentryWebpackPlugin(<omitted for brevity></omitted>)
  ]
}

Expected Result

Instructions how to use sentry-electron in a bundled main thread ESM scenario.

Actual Result

SentryError: Sentry SDK should be initialized before the Electron app 'ready' event is fired

or

TypeError: Cannot read properties of undefined (reading 'require')
    at new Hook (file:///<somepath>/main.bundle.dev.js:37874:40)
    at __webpack_modules__.../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/RequireInTheMiddleSingleton.js.RequireInTheMiddleSingleton._initialize (file:///<somepath>/main.bundle.dev.js:21426:9)
    at new RequireInTheMiddleSingleton (file:///<somepath>/main.bundle.dev.js:21422:14)
    at __webpack_modules__.../node_modules/@opentelemetry/instrumentation/build/esm/platform/node/RequireInTheMiddleSingleton.js.RequireInTheMiddleSingleton.getInstance (file:///<somepath>/main.bundle.dev.js:21479:68)
    at new InstrumentationBase (file:///<somepath>/main.bundle.dev.js:21584:132)
    at new UndiciInstrumentation (file:///<myname>/dev/fugoya/desktop-release/app/dist-dev/main.bundle.dev.js:20399:9)
    at Object.setupOnce (file:///<somepath>/main.bundle.dev.js:57026:31)
    at setupIntegration (file:///<somepath>/main.bundle.dev.js:43076:17)
    at file:///<somepath>/main.bundle.dev.js:43047:7
    at Array.forEach (<anonymous>)

I'm guessing the second approach is correct and I'm just experiencing bundling issues? What is @opentelemetry/instrumentation doing?

tom2strobl commented 1 day ago

I just stumbled across https://github.com/getsentry/sentry-javascript/issues/10940 but am not knowledgeable enough to understand

tom2strobl commented 1 day ago

sorry for the "spam", but for the sake of documentation if others stumble upon this issue and might struggle like me I might as well comment:

I have misunderstood the documentation around asynchronous modules and top level await:

synchronous parts of top level imports are resolved normally in order, just asynchronous parts like top-level-awaits in imported modules are resolved after the entrypoint (eg. main.ts) has finished executing, which can result in awkward timings. If only functions are exported (aka no side-effects), it's perfectly fine to import at the top.
top-level-await in the entry point file is perfectly fine, it's just that you can't await whenReady since that would never finish if the entry point never finishes (it helps finishing reading something)

So since we can't run cli flags, I'm leaning towards the esm without cli flag approach:

/* mainSentry.ts */
import * as Sentry from '@sentry/electron/main'
Sentry.init({ dsn: process.env.SENTRY_DSN })

/* main.ts */
import './mainSentry.ts'
import someotherThings from 'somewhere'

await somePreReadyThings()

app
  .whenReady()
  .then(async () => {
    await somePostReadyThings()
  })
  .catch((e) => {
    // some error handling
  })

So I guess my issue is rather bundling related. I'm assuming that normally people that bundle the main thread still bundle into commonjs or the createRequire style rather than clean esm bundles?

timfish commented 1 day ago

We have a number of bundled examples that are tested but I suspect they all compile to CJS even though some have the code written in ESM. We also have 65+ individual feature tests that use ESM in the main process without bundling.

I guess we need to add another example that bundles to ESM output!

tom2strobl commented 4 hours ago

@timfish Thanks for the swift reply! I should have found the examples in the repo, sorry about that.

You might wonder why output ESM after all when bundling and the reason is that I use a lib that

has natively compiled .node dependencies and therefore cannot be bundled in (have to go down the asar unpack route)
AND is specifically ESM only

so there's not really another way for me I believe. Apart from that I assume over the next couple of years cjs will fade away anyway, so it's probably inevitable sooner or later.

Let me know if I can help!

tom2strobl commented 4 hours ago

By the way enforcing cjs compat import of sentry via

import { createRequire } from 'node:module'
import type * as SentryType from '@sentry/electron/main'
const require = createRequire(import.meta.url)
const Sentry: typeof SentryType = require('@sentry/electron/main')

so far does work as an intermediary solution, gotta check if that is true when bundling for production as I'm afraid import.meta.url is doing static absolute filepaths. The lost types are kinda restored as well with this separate type import hacky business.

timfish commented 3 hours ago

Does createRequire work in a production build too? My guess is that this might only work if you include/package node_modules too because many bundlers don't support resolving createRequire.

Apart from that I assume over the next couple of years cjs will fade away anyway, so it's probably inevitable sooner or later.

Node have recently added support for require(esm) which means a single library will no longer force consumers into ESM mode could slow the migration.

tom2strobl commented 1 hour ago

Node have recently added support for require(esm) which means a single library will no longer force consumers into ESM mode could slow the migration.

Ah interesting, haven't heard about that yet.

So as already assumed createRequire is not a viable solution. It's actually worse than I thought as it bundles the full harcoded file:///<path> without any fancy-ness.

I also tried to rename it to mainSentry.cjs to send very clear signals to webpack and oldschool const Sentry = require('@sentry/electron/main') in there, but it's still bundling the esm version of @sentry/electron, because resolution priority of sentry's export map doesn't really change through the file extension. But I also don't want to change resolve.conditionNames because I do want to favor esm for everything else.

So something I'm trying right now is to mess with the externals configuration and then supplying outside of the bundle:

const commonjsExternals = ['@opentelemetry/instrumentation']

export const electronMainExternals = async function externals({ request }: ExternalItemFunctionData) {
  if (request) {
    if (commonjsExternals.includes(request)) {
      return Promise.resolve(`node-commonjs ${request}`)
    }
    // electronMainExternalsList is a list of electron apis, libs with native deps and built-in-modules
    const isBuiltIn = request.startsWith('node:') || electronMainExternalsList.includes(request)
    if (isBuiltIn) {
      return Promise.resolve(`module ${request}`)
    }
  }
  return false
}

This bundles everything but @opentelementry/instrumentation (which is the outlier) and instructs webpack to properly consume it as cjs (core_1 is bundled and referenced by filepath key and instrumentation_1 is referenced by bare module name, which should resolve fine if I can supply it in the final asar):

It works in dev so far and I think I should be able to supply @opentelementry/instrumentation in prod unbundled just like I do with the libs that have native dependencies.

getsentry / sentry-electron