feat(nextjs): Deprecate Webpack top-level options #18343
Conversation
RulaKhaled
left a comment
RulaKhaled
left a comment
There was a problem hiding this comment.
Generally looks good to me, but i'll recommend waiting for @charly's review!
…level ones for deprecation
…dd deprecation warnings for top-level options
logaretm
force-pushed
the
awad/js-1111-deprecate-top-level-webpack-options
branch
from
beb7c72 to
9ef882c
Compare
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Looks good already, let's make sure to have a docs pr in place that we can deploy immediately when this is released otherwise users will have a conflict between docs|sdk
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Just blocking this for widenClientFileUpload so this does not get merged accidentally
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
|
@chargome I got a companion docs PR ready as well for this so this should be ready to go if you think so. |
There was a problem hiding this comment.
Pull request overview
This PR deprecates webpack-specific configuration options at the top level of Sentry build options and introduces a new webpack namespace to better organize these settings. The change is non-breaking due to a comprehensive backward compatibility layer that migrates deprecated options while emitting deprecation warnings.
- Adds a new
SentryBuildWebpackOptionstype containing all webpack-specific configuration - Implements automatic migration from deprecated top-level options to the new
webpack.*path - Updates all internal references to use the new namespaced configuration
- Maintains full backward compatibility with deprecation warnings for future removal
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nextjs/src/config/types.ts | Defines new SentryBuildWebpackOptions type and adds @deprecated annotations to top-level webpack options; introduces new webpack property on SentryBuildOptions |
| packages/nextjs/src/config/withSentryConfig.ts | Implements migrateDeprecatedWebpackOptions() function to handle backward compatibility and emit deprecation warnings; updates webpack config construction to use new namespaced options |
| packages/nextjs/src/config/webpack.ts | Updates all references from top-level options to webpack.* namespaced options |
| packages/nextjs/src/config/getBuildPluginOptions.ts | Updates plugin option extraction to read from webpack.* namespace |
| packages/nextjs/test/config/withSentryConfig.test.ts | Adds comprehensive test coverage for migration logic, precedence rules, and deprecation warnings |
| packages/nextjs/test/config/getBuildPluginOptions.test.ts | Updates existing tests to use new webpack.* namespaced options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/nextjs/src/config/types.ts
Outdated
| */ | ||
| treeshake?: { | ||
| /** | ||
| * Tree shakes Sentry SDK logger statements from the bundle. Note that this doesn't affect Sentry Logs. |
There was a problem hiding this comment.
The documentation should be more explicit about what setting this option to true does. Consider updating it to:
/**
* When set to `true`, tree shakes Sentry SDK logger statements from the bundle, reducing bundle size.
* Note that this doesn't affect Sentry Logs.
* Defaults to `false`.
*/
debugLogging?: boolean;This makes it clear that setting it to true enables the tree-shaking (removal) behavior.
| * Tree shakes Sentry SDK logger statements from the bundle. Note that this doesn't affect Sentry Logs. | |
| * When set to `true`, tree shakes Sentry SDK logger statements from the bundle, reducing bundle size. | |
| * Note that this doesn't affect Sentry Logs. | |
| * Defaults to `false`. |
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Looks good, just double checking: did you test this out locally and confirmed the webpack options get set correctly? We can't break existing setups with this one
|
Yep, I tested every option and made sure they are set correctly during build, will just fix a quick comment and merge. |
This PR deprecates the webpack-only configurations via the
@deprecatedJSDoc annotation and introduces a newwebpackconfig namespace for them.Under the hood the logic was changed to read from the new values, with a compatibility layer that sets them from the deprecated top-level options while warning for each option if used.
This should set us up for a v11/v12 deletion of those options.
I might have missed a few options that only affect webpack, so I appreciate a good look at this. At any case this isn't breaking so even if missed a few, users won't experience disruptions.