-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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): Widen scope for client file upload #4705
Conversation
size-limit report
|
@iker-barriocanal - since Abhi is out, mind taking a quick look at 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.
The code looks good. A nit: use more descriptive test names, "...has the correct value..." doesn't tell much about the test when it fails; for example, "excludes next's core files" is more descriptive.
My first question by reading the PR description is whether we want the SDK to behave that way - small scope by default and optionally increase it vs big scope by default and optionally increase it? In terms of UX this means enabling an option for an improvement vs enabling it to do what's expected from the SDK(?). Not familiar with other trade-offs, so your call.
What do you think about allowing config to users by allowing them to provide regex/string array/glob instead? This allows them to more finely grained allowlist files, instead of just a generic widen vs. our specific allowlist. |
Allowing users to provide an allowlist would be ideal. My initial concern is that the SDK should work out of the box, and then provide alternatives for users who want specific use cases - it's easy and straightforward to get started and everything works, and you have these options that improve your use case. If you think about new issues, they should be closer to "I have X use case and want to improve it so that I get the benefit Y" than "I just installed the SDK and X doesn't work". |
Indeed. The standard, straightforward case is that all user code ends up in
Would love to, but here is what the output from a typical nextjs build (a.k.a., the files we have to decide whether or not to upload) looks like: You can see the problem. The named files at the bottom are always generated, and are only ever nextjs or third-party code, which is why they're excluded, but the rest...
The reason not to go big -> small is that the majority of what will get uploaded under the "big" scenario contains no user code and therefore really shouldn't be uploaded at all (which is where the push for that PR I linked above came from in the first place). Because there's no (easy) way to tell which files should get uploaded and which files shouldn't, the only way to get the good ones is to take them all, and we don't want to do that if we don't have to - it's a waste of build time for them and a waste of storage for us. (The less easy, doesn't-fit-into-the-roadmap-right-now, not-even-dead-sure-we'd-have-access-to-the-right-files-at-the-right-times way is to load all of the sourcemaps, checkout out their respective I actually would love to try implementing the less easy way at some point (in which case I'd be much more open to switching the default from "small" to "big"), but for now I'm going to merge this because at least it is an improvement from the current state. |
This documents the option added in getsentry/sentry-javascript#4705, which allows users to choose to cast a wider net when uploading sourcemaps for client-side code.
When nextjs builds an app, compiled versions of each page end up in
.next/static/chunks/pages
. In some circumstances, however, user-generated code ends up outside ofpages/
, and in those cases sourcemaps are broken because the relevant files aren't being uploaded.This fixes that by optionally widening the scope of the upload, while excluding files known to contain only nextjs and webpack code. (Every build will generate other files which should be excluded, but there's no way to tell in advance what they're going to be called.) In order to reduce the number of irrelevant files being uploaded to Sentry, this behavior is off by default, but can be turned on with a new option
widenClientFileUpload
:This change is documented in getsentry/sentry-docs#4827.
Fixes #3896
Ref: https://getsentry.atlassian.net/browse/WEB-254