Repository navigation
chore: Switch to rolldown - #20775
Conversation
size-limit report 📦Compared: 538d6e6 (PR test merge using develop at edae47f) Against: edae47f (develop) · base run #65716 ✅ Baseline matches: this diff contains only this PR's changes.
|
64ea470 to
bf6f465
Compare
1f4024d to
7d12000
Compare
e3dab2d to
5104f9a
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
7e9a9c6 to
c823474
Compare
7dc52cf to
9270688
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
just a few small questions/comments nothing blocking from my side, very nice work
| "build:watch": "run-p build:transpile:watch", | ||
| "build:dev:watch": "run-p build:transpile:watch", | ||
| "build:transpile:watch": "rollup -c rollup.npm.config.mjs --watch", | ||
| "build:watch": "run-p build:transpile:watch build:types:watch", |
There was a problem hiding this comment.
q/l: in a lot of the packages we add build:types:watch but don't add it to build:watch as we do here, seems inconsistent is that intentional?
There was a problem hiding this comment.
Wasn't intentional, probably forgot to revert it before we migrated to ts 7.0
| @@ -0,0 +1,5 @@ | |||
| // Called from a served script so the synthetic stacktrace keeps a frame with a URL. Frames from | |||
There was a problem hiding this comment.
q: all the tests you added are regressions against stuff that was fixed upstream at some point?
There was a problem hiding this comment.
Nope, so these specifically are breaking our assertions due to a side effect on the output shape. This specifically affects webkit:
// rollup: anonymous function, no name in the stack
t.captureMessage=function(t,e){...;return Xt().captureMessage(t,n,r)}
// rolldown: named function
function captureMessage(e,t){...;return F().captureMessage(e,n,r)}So what happens is our assertions use stripSentryFramesAndReverse to strip out sentry frames, in rollup one frame survives because it is anonymous, in rolldown that will get stripped so it is a fix to Safari actually, it just passed our assertions accidentally.
Firefox and Chrome infer the anonymous function name from the assignment so they didn't have this side effect.
9270688 to
3908c8e
Compare
| gaugeSpy = spyOn(metrics, 'gauge').mockImplementation(() => undefined); | ||
| countSpy = spyOn(metrics, 'count').mockImplementation(() => undefined); |
There was a problem hiding this comment.
i guess this means users also won't be able to do vi.spyOn(Sentry.metrics, 'count') and similar? clanker suggested that we could export an object literal instead of export * as metrics from './metrics/public-api' and export * as logger from './logger/public-api. wdyt?
There was a problem hiding this comment.
Right, on one hand i don't expect users to mock our APIs during testing, also this has been always true for ESM so this case only affects CJS. So I was hesitant to address this.
But since you raised it, we are shipping this in a minor, so might as well do it to avoid subtle changes. Thanks!
3908c8e to
2cfcf6c
Compare
2cfcf6c to
3c4cb5d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3c4cb5d. Configure here.
3c4cb5d to
3573ba6
Compare
3573ba6 to
9bf6182
Compare
Refreshed what we had in #18156 with the latest rolldown and let's see if we can get this one out.
Benchmarks
Individual SDKs
@sentry/browser(incl. CDN bundles)@sentry/replay-internal(incl. CDN bundles)@sentry/node@sentry/core@sentry/nextjs*Full Build