feat(nextjs): added webpack treeshaking flags as config #18359
Conversation
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
logaretm
force-pushed
the
awad/js-1222-add-treeshaking-options-for-webpack
branch
2 times, most recently
from
bf5a673 to
66aa19c
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the Sentry Next.js SDK's tree-shaking capabilities by renaming debugLogs to debugLogging and introducing four new configuration options for fine-grained control over SDK bundle inclusion.
Key Changes:
- Renamed configuration option from
debugLogstodebugLoggingfor consistency - Added
tracing,excludeReplayIframe,excludeReplayShadowDOM, andexcludeReplayCompressionWorkertree-shaking options - Refactored webpack configuration to use a dedicated
setupTreeshakingFromConfigfunction for improved maintainability
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| packages/nextjs/src/config/types.ts | Added type definitions and JSDoc documentation for four new tree-shaking options |
| packages/nextjs/src/config/webpack.ts | Extracted tree-shaking setup into a dedicated function that handles all five tree-shaking flags via webpack DefinePlugin |
| packages/nextjs/test/config/fixtures.ts | Updated DefinePlugin mock to accept and store definitions for test verification |
| packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts | Added comprehensive test suite covering all tree-shaking options across different build contexts (server, client, edge) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/nextjs/src/config/types.ts
Outdated
| tracing?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code related to capturing iframe content with Session Replay. |
There was a problem hiding this comment.
The phrase "Replacing this flag with true" is awkward. This should be "Setting this to true" to match the pattern used for the tracing option and to be clearer about the action being taken.
packages/nextjs/src/config/types.ts
Outdated
| tracing?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code related to capturing iframe content with Session Replay. |
There was a problem hiding this comment.
Inconsistent hyphenation: "tree-shake" (hyphenated) is used in line 117, but "tree shake" (no hyphen) is used in lines 122, 129, and 137. For consistency, use "tree-shake" (with hyphen) as a verb throughout the documentation.
packages/nextjs/src/config/types.ts
Outdated
| excludeReplayIframe?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code related to capturing shadow dom elements with Session Replay. |
There was a problem hiding this comment.
Inconsistent hyphenation: "tree-shake" (hyphenated) is used in line 117, but "tree shake" (no hyphen) is used here. For consistency, use "tree-shake" (with hyphen) as a verb throughout the documentation.
packages/nextjs/src/config/types.ts
Outdated
| excludeReplayShadowDOM?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code that's related to the included compression web worker for Session Replay. |
There was a problem hiding this comment.
Inconsistent hyphenation: "tree-shake" (hyphenated) is used in line 117, but "tree shake" (no hyphen) is used here. For consistency, use "tree-shake" (with hyphen) as a verb throughout the documentation.
packages/nextjs/src/config/types.ts
Outdated
| excludeReplayIframe?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code related to capturing shadow dom elements with Session Replay. |
There was a problem hiding this comment.
The phrase "Replacing this flag with true" is awkward. This should be "Setting this to true" to match the pattern used for the tracing option and to be clearer about the action being taken.
packages/nextjs/src/config/types.ts
Outdated
| excludeReplayShadowDOM?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code that's related to the included compression web worker for Session Replay. |
There was a problem hiding this comment.
The phrase "Replacing this flag with true" is awkward. This should be "Setting this to true" to match the pattern used for the tracing option and to be clearer about the action being taken.
packages/nextjs/src/config/types.ts
Outdated
| excludeReplayShadowDOM?: boolean; | ||
|
|
||
| /** | ||
| * Replacing this flag with true will tree shake any SDK code that's related to the included compression web worker for Session Replay. |
There was a problem hiding this comment.
The phrase "SDK code that's related to" uses a contraction which is inconsistent with the documentation style of the other options. Change "that's" to "that is" for consistency and professional documentation style.
| * Replacing this flag with true will tree shake any SDK code that's related to the included compression web worker for Session Replay. | |
| * Replacing this flag with true will tree shake any SDK code that is related to the included compression web worker for Session Replay. |
| * Replacing this flag with true will tree shake any SDK code that's related to the included compression web worker for Session Replay. | ||
| * It's only relevant when using Session Replay. | ||
| * Enable this flag if you want to host a compression worker yourself. | ||
| * See Using a Custom Compression Worker for details. |
There was a problem hiding this comment.
The phrase "See Using a Custom Compression Worker for details" is unclear. It should either provide a link or clarify where this documentation can be found (e.g., "See the Sentry documentation on using a custom compression worker for details" or provide an actual URL).
| * See Using a Custom Compression Worker for details. | |
| * See the Sentry documentation on using a custom compression worker for details: https://docs.sentry.io/platforms/javascript/session-replay/custom-compression-worker/ |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
logaretm
force-pushed
the
awad/js-1222-add-treeshaking-options-for-webpack
branch
from
03e070a to
e66a724
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| sentryBuildTimeOptions: { | ||
| webpack: { | ||
| treeshake: { | ||
| debugLogging: true, |
There was a problem hiding this comment.
Bug: Tests use debugLogging and tracing instead of removeDebugLogging and removeTracing for webpack treeshaking, causing silent feature failure.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The tests in packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts incorrectly use debugLogging and tracing properties for webpack treeshaking configuration. The type definitions and implementation correctly expect removeDebugLogging and removeTracing. This mismatch causes the tests to pass without validating the correct configuration, leading users to apply the wrong property names, which results in the treeshaking feature silently failing to function.
💡 Suggested Fix
Update packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts to use removeDebugLogging and removeTracing instead of debugLogging and tracing for webpack treeshaking options.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts#L462
Potential issue: The tests in
`packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts` incorrectly use
`debugLogging` and `tracing` properties for webpack treeshaking configuration. The type
definitions and implementation correctly expect `removeDebugLogging` and
`removeTracing`. This mismatch causes the tests to pass without validating the correct
configuration, leading users to apply the wrong property names, which results in the
treeshaking feature silently failing to function.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5744424
| sentryBuildTimeOptions: { | ||
| webpack: { | ||
| treeshake: { | ||
| debugLogging: true, |
There was a problem hiding this comment.
Bug: Tests use wrong property names for treeshake options
The tests use debugLogging and tracing as property names, but the type definition in types.ts and the implementation in webpack.ts expect removeDebugLogging and removeTracing. Because the tests pass properties that don't exist in the expected interface, the implementation will never set the corresponding DefinePlugin flags, meaning these tests don't actually verify the functionality they claim to test. This affects multiple test cases throughout the new test suite.
This PR updates the Sentry Next.js integration to improve tree-shaking configuration and add new options for finer control over what SDK code is included in the final bundle.
The most significant changes are:
debugLogsoption todebugLogging