tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.81k stars 1.07k forks source link

Fix crash when using `instanceof HTMLElement` in some environments #3494

Closed RobinMalfait closed 4 days ago

RobinMalfait commented 4 days ago

This PR fixes an issue where in some environments where HTMLElement is not available (on the server) and AG Grid is used, we crashed.

This happens because the HTMLElement is polyfilled to an empty object. This means that the typeof HTMLElement !== 'undefined' check passed, but the v instanceof HTMLElement translated to v instanceof {} which is invalid and resulted in a crash...

This PR solves it by checking for exactly what we need, in this case whether the outerHTML property is available.

Alternatively, we could use return v?.outerHTML ?? v, but not sure if that's always safe to do.

Fixes: #3471

vercel[bot] commented 4 days ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2024 9:35am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2024 9:35am
RobinMalfait commented 4 days ago

@thecrypticace thoughts on this solution compared to v?.outerHTML ?? v?

thecrypticace commented 4 days ago

Honestly I think I'd use v?.outerHTML ?? v — just throw in a comment as to why because I know we'll forget.

Aside: If AG Grid is polyfilling HTMLElement to {} and not at least function HTMLElement() {} that is absolutely a bug in AG Grid.

thecrypticace commented 4 days ago

I guess the only reason it wouldn't be safe is if String.prototype.outerHTML was defined right? But that would be really bizarre. So I guess we could check for an object. I'm fine with either honestly.

RobinMalfait commented 4 days ago

Aside: If AG Grid is polyfilling HTMLElement to {} and not at least function HTMLElement() {} that is absolutely a bug in AG Grid.

100% agree, if they do that for test code in their own tests that's one thing. But this is in production, so definitely not ideal.

Either way, with this PR the code is a bit cleaner as well so why not..