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

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

Closed
3 tasks done
tom2strobl opened this issue Oct 31, 2024 · 10 comments
Closed
3 tasks done
Assignees

Comments

@tom2strobl
Copy link

tom2strobl commented Oct 31, 2024

Is there an existing issue for this?

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
Copy link
Author

I just stumbled across getsentry/sentry-javascript#10940 but am not knowledgeable enough to understand

@tom2strobl
Copy link
Author

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
Copy link
Collaborator

timfish commented Oct 31, 2024

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
Copy link
Author

@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!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Nov 1, 2024
@tom2strobl
Copy link
Author

tom2strobl commented Nov 1, 2024

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
Copy link
Collaborator

timfish commented Nov 1, 2024

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
Copy link
Author

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):
Image

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.

@timfish
Copy link
Collaborator

timfish commented Nov 7, 2024

With the following webpack config, I can build working ESM code for the Electron main process:

  {
    mode: 'production',
    entry: './src/main.js',
    target: 'electron-main',
    output: {
      library: { type: 'module' },
      chunkFormat: 'module',
      module: true,
      filename: 'main.mjs',
    },
    experiments: {
      topLevelAwait: true,
      outputModule: true
    },
    plugins: [sentryWebpackPlugin(sentryWebpackPluginOptions)],
  },

This does result in a couple of webpack build time warnings about Critical dependency: the request of a dependency is an expression around @opentelemetry/instrumentation. These are just warnings and only impact auto instrumentation of libraries for Sentry performance features. Auto instrumentation is generally only used when using the Sentry SDKs server side to capture database spans.

@tom2strobl
Copy link
Author

I disregarded the most obvious option target: electron-main long time ago as I ran into issues with dynamic imports that weren't transpiled nicely. Turns out going down the es2022 path with manual externals was the actual footgun that overcomplicated things. I just made a minimal reproduction repo and did not find any issues with dynamic imports anymore at all. 🤦🏻‍♂️

Just using target: electron-main (or more specifically electron32.1-main for potential optimizations) makes @sentry/electron bundle nicely and I already had the ignoreWarnings: [{ module: /@opentelemetry\/instrumentation/ }] in the config.

Terribly sorry for not giving the most obvious option a go again and having you waste time on this.
Thanks for the great support as always!

@timfish
Copy link
Collaborator

timfish commented Nov 7, 2024

No worries at all. I've now got a test app to add to the examples!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants