Uh oh!
There was an error while loading. Please reload this page.
ref(core): Remove redundant iframe check in supportsNativeFetch - #19853
Closed
HazAT wants to merge 5 commits into
Closed
ref(core): Remove redundant iframe check in supportsNativeFetch#19853HazAT wants to merge 5 commits into
supportsNativeFetch#19853HazAT wants to merge 5 commits into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
size-limit report 📦
|
Contributor
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.
|
Lms24force-pushed
the
bundle-size/supports-native-fetch
branch
from
March 19, 2026 11:42
e2fd3a8 to
39fcc12ComparesupportsNativeFetchContributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Bug Fixes 🐛Core
Other
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Lms24
changed the base branch from
autoresearch/browser-bundle-size-2026-03-17
to
developMarch 19, 2026 11:49
Lms24force-pushed
the
bundle-size/supports-native-fetch
branch
from
March 19, 2026 12:13
39fcc12 to
dff6335CompareUh oh!
There was an error while loading. Please reload this page.
Lms24
reviewed
Mar 19, 2026
| // eslint-disable-next-line @typescript-eslint/ban-types | ||
| export function isNativeFunction(func: Function): boolean { | ||
| return func && /^function\s+\w+\(\)\s+\{\s+\[native code\]\s+\}$/.test(func.toString()); | ||
| return func && /\[native code\]/.test(func.toString()); |
Member
There was a problem hiding this comment.
I'll revert this. It saves a few bytes but drastically reduces specificity. We have no unit tests that cover this function :(
Comment on lines
-111
to
-141
| if (!_isFetchSupported()) { | ||
| return false; | ||
| } | ||
| // Fast path to avoid DOM I/O | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| if (isNativeFunction(WINDOW.fetch)) { | ||
| return true; | ||
| } | ||
| // window.fetch is implemented, but is polyfilled or already wrapped (e.g: by a chrome extension) | ||
| // so create a "pure" iframe to see if that has native fetch | ||
| let result = false; | ||
| const doc = WINDOW.document; | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| if (doc && typeof (doc.createElement as unknown) === 'function') { | ||
| try { | ||
| const sandbox = doc.createElement('iframe'); | ||
| sandbox.hidden = true; | ||
| doc.head.appendChild(sandbox); | ||
| if (sandbox.contentWindow?.fetch) { | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| result = isNativeFunction(sandbox.contentWindow.fetch); | ||
| } | ||
| doc.head.removeChild(sandbox); | ||
| } catch (err) { | ||
| DEBUG_BUILD && debug.warn('Could not create sandbox iframe for pure fetch check, bailing to window.fetch: ', err); | ||
| } | ||
| } | ||
| return result; |
Member
There was a problem hiding this comment.
Unfortunately, this is a behaviour change to the original function. Supporting afetch implementation is not the same as supporting/having the native one. I'm going to experiment with moving getNativeImplementation to core so that we can actually reuse the iframe lookup but I'm not sure how much bundle size this still saves.
Member
There was a problem hiding this comment.
Saving old size check for comparison:
size-limit report 📦
| Path | Size | % Change | Change |
|---|---|---|---|
| @sentry/browser | 25.4 kB | -0.92% | -234 B 🔽 |
| @sentry/browser - with treeshaking flags | 23.94 kB | -0.84% | -201 B 🔽 |
| @sentry/browser (incl. Tracing) | 42.4 kB | -0.52% | -220 B 🔽 |
| @sentry/browser (incl. Tracing, Profiling) | 47.06 kB | -0.48% | -223 B 🔽 |
| @sentry/browser (incl. Tracing, Replay) | 81.18 kB | -0.31% | -245 B 🔽 |
| @sentry/browser (incl. Tracing, Replay) - with treeshaking flags | 70.78 kB | -0.3% | -211 B 🔽 |
| @sentry/browser (incl. Tracing, Replay with Canvas) | 85.88 kB | -0.29% | -245 B 🔽 |
| @sentry/browser (incl. Tracing, Replay, Feedback) | 98.14 kB | -0.24% | -235 B 🔽 |
| @sentry/browser (incl. Feedback) | 42.22 kB | -0.53% | -221 B 🔽 |
| @sentry/browser (incl. sendFeedback) | 30.07 kB | -0.78% | -234 B 🔽 |
| @sentry/browser (incl. FeedbackAsync) | 35.13 kB | -0.66% | -232 B 🔽 |
| @sentry/browser (incl. Metrics) | 26.68 kB | -0.92% | -246 B 🔽 |
| @sentry/browser (incl. Logs) | 26.82 kB | -0.91% | -245 B 🔽 |
| @sentry/browser (incl. Metrics & Logs) | 27.51 kB | -0.83% | -228 B 🔽 |
| @sentry/react | 27.19 kB | -0.75% | -203 B 🔽 |
| @sentry/react (incl. Tracing) | 44.73 kB | -0.5% | -224 B 🔽 |
| @sentry/vue | 29.86 kB | -0.73% | -218 B 🔽 |
| @sentry/vue (incl. Tracing) | 44.26 kB | -0.51% | -226 B 🔽 |
| @sentry/svelte | 25.43 kB | -0.93% | -238 B 🔽 |
| CDN Bundle | 28.11 kB | -0.61% | -172 B 🔽 |
| CDN Bundle (incl. Tracing) | 43.29 kB | -0.49% | -213 B 🔽 |
| CDN Bundle (incl. Logs, Metrics) | 28.97 kB | -0.62% | -178 B 🔽 |
| CDN Bundle (incl. Tracing, Logs, Metrics) | 44.16 kB | -0.44% | -195 B 🔽 |
| CDN Bundle (incl. Replay, Logs, Metrics) | 68.03 kB | -0.27% | -184 B 🔽 |
| CDN Bundle (incl. Tracing, Replay) | 80.15 kB | -0.23% | -179 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Logs, Metrics) | 81.04 kB | -0.24% | -193 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback) | 85.66 kB | -0.25% | -211 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics) | 86.55 kB | -0.25% | -215 B 🔽 |
| CDN Bundle - uncompressed | 81.97 kB | -0.8% | -655 B 🔽 |
| CDN Bundle (incl. Tracing) - uncompressed | 127.89 kB | -0.52% | -666 B 🔽 |
| CDN Bundle (incl. Logs, Metrics) - uncompressed | 84.83 kB | -0.77% | -655 B 🔽 |
| CDN Bundle (incl. Tracing, Logs, Metrics) - uncompressed | 130.76 kB | -0.51% | -666 B 🔽 |
| CDN Bundle (incl. Replay, Logs, Metrics) - uncompressed | 208.47 kB | -0.32% | -655 B 🔽 |
| CDN Bundle (incl. Tracing, Replay) - uncompressed | 244.75 kB | -0.28% | -666 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Logs, Metrics) - uncompressed | 247.6 kB | -0.27% | -666 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 257.66 kB | -0.26% | -666 B 🔽 |
| CDN Bundle (incl. Tracing, Replay, Feedback, Logs, Metrics) - uncompressed | 260.5 kB | -0.26% | -666 B 🔽 |
| @sentry/nextjs (client) | 47.16 kB | -0.44% | -204 B 🔽 |
| @sentry/sveltekit (client) | 42.82 kB | -0.6% | -255 B 🔽 |
| @sentry/node-core | 56.28 kB | -0.12% | -63 B 🔽 |
| @sentry/node | 173.21 kB | +0.03% | +51 B 🔺 |
| @sentry/node - without tracing | 96.27 kB | -0.08% | -76 B 🔽 |
| @sentry/aws-serverless | 113.28 kB | -0.06% | -62 B 🔽 |
Uh oh!
There was an error while loading. Please reload this page.
Lms24
commented
Mar 19, 2026
Member
trying one more approach here to only extract the iframe stuff |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Lms24
reviewed
Mar 20, 2026
Comment on lines
+48
to
+61
| // This is a really weird fallback but here's what's going on: | ||
| // We're just being extra careful here. According to types, windowImpl is _always_ defined. | ||
| // However, in some very rare cases (for example test environments), it may in fact not be defined. | ||
| // In exactly this case, if we fail to get an iframeImpl, AND no windowImpl either, | ||
| // we skip caching and just return effectively undefined (despite types saying it's always defined) | ||
| // This basically tricks TS into thinking this function never returns `undefined` which | ||
| // for the most part is true. | ||
| if (!windowImpl) { | ||
| return windowImpl; // but actually return undefined | ||
| } | ||
| return (cachedImplementations[name] = impl.bind(WINDOW) as CacheableImplementations[T]); | ||
| // If _only_ iframeImpl is undefined and windowImpl is defined and not not native, we end up here | ||
| // In this case, we deliberately cache the windowImpl. | ||
| return cacheAndReturn(windowImpl); |
Member
There was a problem hiding this comment.
Writing this down in case we ever wonder why the hell this code path exists:
This PR DID NOT change any prior behavior. I think this function was already flawed but for the scope of this PR, I'll not fix it right away. Two sketchy things:
- We have a code path where we effectively return undefined, despite TS assuming we always return a defined implementation (see 1st comment why)
- We return and cache a non-native, potentially patched version of an implementation in a function that's called
getNativeImplementation.
This PR just more clearly call out this behaviour as it was quite hidden away in the prior implementation. I'll investigate if we can fix at least the latter flaw by returning undefined.
supportsNativeFetch() created a sandboxed iframe to check if the Fetch API
is natively implemented — identical logic to getNativeImplementation('fetch')
in browser-utils. The iframe approach adds ~30 lines of DOM manipulation code.
The function is only called behind a `skipNativeFetchCheck` guard that is
never set to true in the base CDN bundle, making the iframe code dead weight
that cannot be tree-shaken by terser (it can't prove the parameter is always
falsy across the program).
Simplify to delegate to `_isFetchSupported()` which checks if `window.fetch`
exists. The actual native-vs-polyfill distinction is already handled by
`getNativeImplementation` at the call sites that need it.
Also simplifies the `isNativeFunction` regex from an exact whitespace pattern
to a simpler `/[native code]/` check, which is more permissive across
different JS engine toString() formats.
Saves ~200 bytes gzipped on the base browser bundle.
Co-Authored-By: Claude claude@anthropic.comLms24force-pushed
the
bundle-size/supports-native-fetch
branch
from
March 20, 2026 12:24
416056a to
f7c95acCompareLms24
commented
Mar 20, 2026
Member
lol, so after making far to many changes to still get this in without behaviour changes but less duplication, I ended up with a larger bundle size than before. Code is arguably cleaner now but honestly not worth pursuing further. Closing this experiment 😅 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remove ~30 lines of iframe-based DOM manipulation from
supportsNativeFetch(). Saves ~200 bytes gzipped — the single biggest win in this bundle size effort.Problem
supportsNativeFetch()created a sandboxed iframe to check iffetchis natively implemented. This is identical logic togetNativeImplementation("fetch")inbrowser-utils, creating two separate iframe-based checks in the CDN bundle.The function is only called behind a
skipNativeFetchCheckguard:In the base CDN bundle,
skipNativeFetchCheckis never set totrue, making the iframe code dead weight. Terser cannot tree-shake it because it cannot prove the parameter is always falsy.Solution
Simplify
supportsNativeFetch()to just delegate to_isFetchSupported()(checks ifwindow.fetchexists). The native-vs-polyfill distinction is handled bygetNativeImplementationat call sites that need it.Also simplifies
isNativeFunctionfrom/^function\s+\w+\(\)\s+\{\s+\[native code\]\s+\}$/to/\[native code\]/— more permissive across different JS enginetoString()formats.Part of #19833.
Co-Authored-By: Claude claude@anthropic.com