-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash when using instanceof HTMLElement
in some environments
#3494
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@thecrypticace thoughts on this solution compared to |
Honestly I think I'd use Aside: If AG Grid is polyfilling HTMLElement to |
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. |
Some tools, like AG Grid, polyfill `HTMLElement` 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... This hopefully solves it. Alternatively, we could use `return v?.outerHTML ?? v`, but not sure if that's always safe to do.
4888d9d
to
ddef5c7
Compare
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.. |
When will it be released? |
@NPJigaK hey this has been released in the latest version. You can upgrade using |
This PR fixes an issue where in some environments where
HTMLElement
is notavailable (on the server) and AG Grid is used, we crashed.
This happens because the
HTMLElement
is polyfilled to an empty object. This means that thetypeof HTMLElement !== 'undefined'
check passed, but thev instanceof HTMLElement
translated tov 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