Skip to content

Conversation

@Florent-Bouisset
Copy link
Collaborator

As @peaBerberian noticed, applications can install polyfills that override the fetch method on the window object.
However, in our experience, these polyfill implementations have not been very reliable.

The goal of this PR is to detect whether the fetch method has been polyfilled. If it has been overridden, we fall back to using XHR instead.

This detection uses the toString method, which typically returns [native code] for unmodified native functions.

I'm still unsure whether it's the responsibility of RxPlayer to check for the absence of a polyfill, or if it's up to the application to ensure everything works as expected.

@github-actions
Copy link

✅ Automated performance checks have passed on commit 0067dd6cb522140f740fac4350121f55cdf4adf0 with the base branch dev.

Details

Performance tests 1st run output

No significative change in performance for tests:

Name Mean Median
loading 20.25ms -> 20.49ms (-0.240ms, z: 1.38034) 27.75ms -> 27.90ms
seeking 17.36ms -> 20.84ms (-3.479ms, z: 0.40302) 11.25ms -> 11.25ms
audio-track-reload 27.15ms -> 27.16ms (-0.015ms, z: 0.08417) 39.45ms -> 39.45ms
cold loading multithread 49.47ms -> 48.04ms (1.430ms, z: 10.41485) 71.70ms -> 70.50ms
seeking multithread 32.65ms -> 32.63ms (0.025ms, z: 0.98301) 10.35ms -> 10.35ms
audio-track-reload multithread 26.79ms -> 26.84ms (-0.055ms, z: 0.34145) 39.45ms -> 39.30ms
hot loading multithread 16.46ms -> 16.37ms (0.085ms, z: 1.92426) 23.70ms -> 23.55ms

@peaBerberian
Copy link
Collaborator

peaBerberian commented May 7, 2025

Note that it is also AbortController that we should check. If it isn't the native implementation, it is unlikely to provoke request cancellation, which is the source problem we were having.

Explanation of the issue: TanStack/query#9049

Also checking fetch could make sense though.

@github-actions
Copy link

✅ Automated performance checks have passed on commit 81f7a7f28620e0a767a94929ac4b81c286554890 with the base branch dev.

Details

Performance tests 1st run output

No significative change in performance for tests:

Name Mean Median
loading 19.09ms -> 19.48ms (-0.393ms, z: 1.55153) 26.25ms -> 26.25ms
seeking 13.78ms -> 13.16ms (0.627ms, z: 0.07582) 11.10ms -> 11.10ms
audio-track-reload 26.08ms -> 26.10ms (-0.012ms, z: 0.34079) 37.95ms -> 37.95ms
cold loading multithread 46.50ms -> 45.83ms (0.670ms, z: 11.39122) 67.95ms -> 66.90ms
seeking multithread 23.21ms -> 13.20ms (10.005ms, z: 0.30616) 10.35ms -> 10.35ms
audio-track-reload multithread 25.73ms -> 25.81ms (-0.089ms, z: 0.03937) 37.80ms -> 37.80ms
hot loading multithread 15.00ms -> 14.92ms (0.082ms, z: 4.70462) 21.75ms -> 21.45ms

@github-actions
Copy link

✅ Automated performance checks have passed on commit e6bd4ca62218ab8e934b520a229dd374dbc4e759 with the base branch dev.

Details

Performance tests 1st run output

No significative change in performance for tests:

Name Mean Median
loading 19.79ms -> 19.86ms (-0.074ms, z: 1.08172) 27.15ms -> 27.15ms
seeking 20.08ms -> 16.73ms (3.347ms, z: 1.48010) 11.25ms -> 11.40ms
audio-track-reload 26.76ms -> 26.78ms (-0.020ms, z: 0.01161) 39.00ms -> 39.00ms
cold loading multithread 47.73ms -> 46.87ms (0.861ms, z: 10.09056) 69.90ms -> 68.70ms
seeking multithread 22.67ms -> 22.10ms (0.575ms, z: 1.85886) 10.50ms -> 10.50ms
audio-track-reload multithread 26.61ms -> 26.51ms (0.096ms, z: 2.00305) 39.00ms -> 38.70ms
hot loading multithread 15.74ms -> 15.70ms (0.046ms, z: 3.11287) 22.65ms -> 22.50ms

@peaBerberian
Copy link
Collaborator

peaBerberian commented May 15, 2025

Works as intended on the device I tested.


CAVEAT TO NOTE: As we discussed this is not exactly 100% ECMAScript-compliant with what they call "white spaces" and "line terminators" with how browsers COULD be implementing this.
This means the risks of a false negative is still theoretically possible, but I think a browser should go out of their way to purposefully break code while still being spec-compliant, as most random resources on the internet seems to just rely on a simpler trick like .includes("[native code]").

Also we saw that some irritating polyfill implementations, including from core-js, do include the [native code] (for 100% compliance), but IMO an AbortController implementation which isn't understood by the browsers own API (like fetch) means that it is not 100% compliant and as such, well-written polyfills should hopefully avoid doing that.
I saw that polyfill imported by the applications concerned by this issue did not include [native code] so we're good for them.


Lastly, this check fails when running RxPaired because we're monkey-patching fetch in it :/

We may want to add special characters in RxPaired's fetch implems and in the RxPlayer? Or just not do the RegExp with fetch and assume that a native AbortController (which came much later than the fetch API to browsers and as a DOM API) is a sufficient check?

@Florent-Bouisset
Copy link
Collaborator Author

Works as intended on the device I tested.

CAVEAT TO NOTE: As we discussed this is not exactly 100% ECMAScript-compliant with what they call "white spaces" and "line terminators" with how browsers COULD be implementing this. This means the risks of a false negative is still theoretically possible, but I think a browser should go out of their way to purposefully break code while still being spec-compliant, as most random resources on the internet seems to just rely on a simpler trick like .includes("[native code]").

Also we saw that some irritating polyfill implementations, including from core-js, do include the [native code] (for 100% compliance), but IMO an AbortController implementation which isn't understood by the browsers own API (like fetch) means that it is not 100% compliant and as such, well-written polyfills should hopefully avoid doing that. I saw that polyfill imported by the applications concerned by this issue did not include [native code] so we're good for them.

Lastly, this check fails when running RxPaired because we're monkey-patching fetch in it :/

We may want to add special characters in RxPaired's fetch implems and in the RxPlayer? Or just not do the RegExp with fetch and assume that a native AbortController (which came much later than the fetch API to browsers and as a DOM API) is a sufficient check?

I have update to only check AbortController so we won't have those edge cases.

@github-actions
Copy link

✅ Automated performance checks have passed on commit e5a0509f97b8468bc9dbba5274fff07cba28c49d with the base branch dev.

Details

Performance tests 1st run output

No significative change in performance for tests:

Name Mean Median
loading 20.34ms -> 20.36ms (-0.018ms, z: 0.74910) 27.90ms -> 27.90ms
seeking 24.25ms -> 24.21ms (0.039ms, z: 0.21881) 11.40ms -> 11.40ms
audio-track-reload 27.11ms -> 27.31ms (-0.200ms, z: 2.37544) 39.45ms -> 39.75ms
cold loading multithread 48.49ms -> 47.38ms (1.114ms, z: 12.94324) 70.95ms -> 69.45ms
seeking multithread 21.34ms -> 23.34ms (-2.001ms, z: 1.09647) 10.50ms -> 10.35ms
audio-track-reload multithread 26.88ms -> 26.88ms (0.000ms, z: 0.17883) 39.60ms -> 39.60ms
hot loading multithread 16.19ms -> 16.06ms (0.131ms, z: 2.62181) 23.40ms -> 23.25ms

@peaBerberian peaBerberian merged commit 9dcb99f into dev Jun 16, 2025
10 checks passed
@peaBerberian peaBerberian added this to the 4.4.0 milestone Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants