-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(nextjs): Update code sample in Advanced Usage of Webpack #3677
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/sentry/sentry-docs/4ZTDqF7VsD4mazFeDkxvrJ8dw6K8 |
@@ -77,6 +77,8 @@ Set up the `SentryWebpackPlugin` as the last running plugin, otherwise, the resu | |||
|
|||
If you prefer to upload source maps manually, configure Webpack to output source maps: | |||
|
|||
<PlatformSection notSupported={["javascript.nextjs"]}> |
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 like your thinking on this, but using PlatformSection is a little bit of an unusual approach. Generally, if we want to change a code sample, we build an include
directory and pop the samples in there. If there's a sample for JS and a different sample that's specific to NextJS, that's fine. The JS one acts as the default for JS and its guides, and the NextJS code sample displays only for NextJS.
Let me know if you need help with this.
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.
Moved the samples to another directory, in include/
. Not sure whether this is the best place though since it seems this directory contains content for all the languages and webpack is only for JS. Let me know what you think about it, and I can move the PlatformSection
content above in a different PR.
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.
Not a result of your change, but while we're here...
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.nextjs.mdx
Outdated
Show resolved
Hide resolved
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.mdx
Outdated
Show resolved
Hide resolved
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 think this structure works better.
One nit: The include
directory is very buried. Can we move it to simply configuration/sourcemaps/tools
? Your call. thanks @iker-barriocanal
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.nextjs.mdx
Show resolved
Hide resolved
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.mdx
Show resolved
Hide resolved
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.nextjs.mdx
Show resolved
Hide resolved
src/includes/configuration/sourcemaps/tools/webpack/advanced-usage/javascript.mdx
Show resolved
Hide resolved
Nothing major here, mostly just carryover from comments on #3677 which didn't get in before it was merged. - Use `devtool` in all snippets so that sourcemaps are generated correctly - Fix setting `devtool` in nextjs (set it on `config`) - Remove stray `configureWebpack` wrapper in two snippets - Remove `productionBrowserSourceMaps` from nextjs config since [we no longer use it](getsentry/sentry-javascript#3765) - Remove `SentryWebpackPluginOptions` from nextjs snippet not using plugin
The advanced usage of Webpack was valid in a prior version of the SDK. This PR updates that code sample, besides removing some comments to reduce noise. If a user wants to read that noise or just learn more about it, a link to the
manual-setup
page has been included.