-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Update default ignore list for sourcemaps #18938
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
feat(nextjs): Update default ignore list for sourcemaps #18938
Conversation
…could-not-determine-source-map-reference
| MAIN_CHUNKS: '**/main-*', | ||
| FRAMEWORK_CHUNKS: '**/framework-*', | ||
| FRAMEWORK_CHUNKS_DOT: '**/framework.*', | ||
| POLYFILLS_CHUNKS: '**/polyfills-*', | ||
| WEBPACK_CHUNKS: '**/webpack-*', | ||
| PAGE_CLIENT_REFERENCE_MANIFEST: '**/page_client-reference-manifest.js', | ||
| SERVER_REFERENCE_MANIFEST: '**/server-reference-manifest.js', | ||
| NEXT_FONT_MANIFEST: '**/next-font-manifest.js', | ||
| MIDDLEWARE_BUILD_MANIFEST: '**/middleware-build-manifest.js', | ||
| INTERCEPTION_ROUTE_REWRITE_MANIFEST: '**/interception-route-rewrite-manifest.js', | ||
| ROUTE_CLIENT_REFERENCE_MANIFEST: '**/route_client-reference-manifest.js', | ||
| MIDDLEWARE_REACT_LOADABLE_MANIFEST: '**/middleware-react-loadable-manifest.js', |
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.
h: We need to ensure these are not too widely applied. The way it is now, these ignore patterns are also passed to the server source maps which might lead to false positives and ignore files we don't want to ignore.
We need a way to distinguish between which patterns we pass to which commands.
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 reverted the changes for all framework, polyfills etc. and just kept the manifest ones for now. We need to investigate if we can split up the upload calls or be smarter here. Also with turbopack there are likely more files we can ignore
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| '**/route_client-reference-manifest.js', | ||
| '**/middleware-react-loadable-manifest.js', | ||
| '**/vendor/**', | ||
| ]); |
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.
Missing integration or E2E test for feature PR
Low Severity · Bugbot Rules
This feat PR changes the sourcemap ignore pattern behavior (merging user patterns with defaults instead of replacing) but only includes unit tests. Per the review rules: "When reviewing a feat PR, check if the PR includes at least one integration or E2E test. If neither of the two are present, add a comment, recommending to add one." An integration test verifying the actual sourcemap upload/ignore behavior with the new merged patterns would help ensure the feature works correctly in real build scenarios.
closes #18872