-
Notifications
You must be signed in to change notification settings - Fork 54
added sentry-webpack plugin for source maps #1371
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Only small concern with the sourcemaps
craco.config.js
Outdated
module.exports = function () { | ||
return { | ||
babel: { | ||
plugins: ['@babel/plugin-proposal-nullish-coalescing-operator'], | ||
}, | ||
webpack: { | ||
plugins, | ||
devtool: SENTRY_AUTH_TOKEN ? 'source-map' : '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought CRA would create the sourcemaps automatically unless you disable them https://stackoverflow.com/questions/55904292/how-to-generate-sourcemaps-in-create-react-app
Also, would this be disabling the sourcemap if SENTRY_AUTH_TOKEN is not present? note that if it's enabled by default, and u set '' then probably u are killing source map...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was that if we don't define auth token then we do not need/want source maps but seems like react-scripts ignore this and still create source maps. I wasn't going into too much details in the react-scripts config but seems like they are also using source-map
value for devtool based on this LINK so maybe we can remove this line, what do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a sidenote there is a option to disable source-maps with env variable LINK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is, if it's enabled by default, why do you need to enable it yourself with craco?
But more concerning is, why of there's not SENTRY_AUTH_TOKEN u want to disable it? (instead of leaving the default)
In general, I would think, we always want the sourcemaps. I would leave the default if possible. If we want to disable, we can always use the official ENV var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that line. My understanding was that source maps slow build time and also load time (which seems to not be true since they only load if you open dev tools apparently).
# Summary This will provide source maps support in sentry console which will make easier to debug errors in production.  # To Test - source maps support should be visible in the sentry console after the deployment of source code with this valid sentry authToken # Background - the sentry-webpack-plugin will only be added if the REACT_APP_SENTRY_AUTH_TOKEN is defined as a env variable. The value of this variable is the authToken provided by sentry.
Summary
This will provide source maps support in sentry console which will make easier to debug errors in production.
To Test
Background