Check for moveBefore on instance instead of prototype #33177
Conversation
|
Comparing: 5d04d73...dcc1f19 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Thanks for the thoughtful fix, @devongovett ! Checking for moveBefore on the instance itself instead of the prototype makes a lot of sense, especially considering environments like JSDOM where the prototype might not behave as expected. This change feels like a good safety enhancement without affecting bundle size. Out of curiosity, are there other DOM methods that React relies on via the prototype which might be worth auditing similarly? |
|
The problem is that this makes a one-time check during module eval into a repeated check during commit that needs to check the prototype chain. And we want that to be really fast. So it's a perf sensitive change that impacts everybody just for some exotic cases. Do we have precedent for repeating DOM feature detection repeatedly? I thought we already have other features we detect once instead of repeatedly i.e. we always assume we're dealing with a consistent DOM implementation. |
|
Thanks for the clarification, @eps1lon, that makes sense. Performance during commit is definitely critical, and I see how repeated prototype checks could introduce unnecessary overhead for most users. Given that, would it make sense to conditionally apply the instance check only in non-production or test environments (e.g., when rendering with JSDOM), while keeping the existing behavior in production? Or perhaps document this as a known limitation for alternative DOMs? Just curious if there's room for a compromise that preserves perf while helping those edge-case environments. |
|
The rest of React does not assume that you are rendering into the global window. For example, it uses I'm not sure how best to measure the perf impact here, but I find it unlikely that a |
|
That's a great point about React already avoiding assumptions about the global window using I also agree that a simple Maybe this could open the door for a more general pattern or utility within React for safe feature detection when DOM variations are expected? That way, perf-critical paths could stay lean while still supporting more flexible use cases when needed. Appreciate the work you’re doing here, really interesting conversation!! |
|
I was kind of hesitant to even have the module scope feature flag there instead of a static flag. Maybe we should just revert moveBefore until it's widely available and we can assume it's polyfilled. |
|
Thanks for chiming in, @sebmarkbage . Reverting moveBefore until broader availability and polyfilling is definitely a pragmatic path, It avoids adding branching logic or perf-sensitive checks for now. If the goal is to keep the codebase streamlined while still supporting edge cases like JSDOM, maybe a documented fallback strategy or official guidance for polyfilling moveBefore in non-standard environments could help? That might give library authors and test runners a clear path without complicating the core runtime. Curious to hear what you and the team lean toward! |
The feature detection for Element#moveBefore is currently done using
window.Element.prototype, however it is possible to render into a different DOM implementation that does not support this. For example, one could render or portal into a JSDOM instance. It's safer to check whether the actual instance has this method before calling it rather than checking the global prototype.Reproduction: https://codesandbox.io/p/sandbox/nl3jnk
This also affects React Aria Components which does something similar to implement collections. See adobe/react-spectrum#8220 and vercel/next.js#79069.