zivl / sentry-testkit

A Sentry plugin to allow Sentry report interception and further inspection of the data being sent
https://zivl.github.io/sentry-testkit/
MIT License
112 stars 26 forks source link

3.1.0. Module not found: Can't resolve 'net' in 'xxxx/node_modules/express/lib' #43

Closed nghiepdev closed 4 years ago

nghiepdev commented 4 years ago

I just upgraded to 3.1.0 encountered an error

Module not found: Can't resolve 'net' in 'xxxx/node_modules/express/lib'
zivl commented 4 years ago

Adding @ohana54

ohana54 commented 4 years ago

@nghiepit I'm not sure why you get that. My PR added the express library, which relies on the built-in node package net. Can you share some info on how you use sentry-testkit? Which environment, node version etc.?

nghiepdev commented 4 years ago

@ohana54 I'm using Next.js. Currently this example https://github.com/zeit/next.js/tree/canary/examples/with-sentry also cause the error. Module not found: Can't resolve 'fs' in 'xxx/with-sentry-app/node_modules/destroy'

You can clone the example and upgrade sentry-testkit to 3.1.0

ohana54 commented 4 years ago

It seems that next.js is loading the testkit in the browser as well, in order to not send event when in development mode. That's why you get the exception.

I think the testkit was meant to be used in a node environment only (@zivl correct me if I'm wrong), and up until now there were no node dependencies so it worked by accident.

There's a different way to filter events in development, according to the docs: https://docs.sentry.io/platforms/javascript/#filter-events--custom-logic

Example with next.js code:

  if (process.env.NODE_ENV !== 'production') {
    // Don't actually send the errors to Sentry
    sentryOptions.beforeSend = () => null

    // Instead, dump the errors to the console
    sentryOptions.integrations = [
      new SentryIntegrations.Debug({
        // Trigger DevTools debugger instead of using console.log
        debugger: false,
      }),
    ]
  }

  Sentry.init(sentryOptions)

@zivl WDYT about this?

zivl commented 4 years ago

@ohana54 yes sentry-testkit was not design to work in browser environment. I kinda wonder WHY it is not working though? not sure I 100% understand the issue. Also, next.js are using sentry-testkit@2 and I wonder if they can benefit more from the recent features of sentry-testkit

ohana54 commented 4 years ago

@zivl it's not working because now we depend on express which depends on native node modules, which are not present in the browser.

@nghiepit Assuming you have your own code integrating with Sentry, can you use the solution I proposed?

nghiepdev commented 4 years ago

@ohana54 Thank for your proposal. But I chose to downgrade to ~3.0.1 to resolve quickly my problem. Maybe the problem has related with hot-reloader https://github.com/zeit/next.js/issues/9768

zivl commented 4 years ago

@ohana54 thinking out loud, catching reports from another process is pretty fresh and not the common way of using sentry-testkit, so how about importing express as peerDependency? this way it will be on the consumer's own responsibility, so only if you really need it - you will have to make sure express is there?

ohana54 commented 4 years ago

Express is an internal implementation, so I don't think it should be a peer dependency (which will make it public API)