Avoid instanceof #621
Avoid instanceof #621
Conversation
There was a problem hiding this comment.
Hey, thanks for this PR and highlighting this issue.
My concerns lie with false positives of class types within Pixi and ensureing we are being exhaustive of all subclasses of container.
In languages without runtime type checking, the simplest way to resolve this issue is to tag or brand the class with data to do the comparison reliably.
We should move forward with this PR (after review) to unblock, and draft a PR to core pixi to add a fallback for type checking accross realms benifiting the greater pixi community as well -- A reasonably reliable solution that works across realms and would have low false positives would be to add a static type field to the core pixi classes that uses Symbol.for to create a global symbol for each pixi class, then walk the prototype chain and compare instance.constructor.type to the global symbol for a container.
src/helpers/typeChecks.ts
Outdated
| return typeof obj.nodeName === 'string' | ||
| && typeof obj.nodeType === 'number' | ||
| && obj.nodeType === 1 // Element node | ||
| && typeof obj.style === 'object'; |
There was a problem hiding this comment.
Would this check suffice?
return obj.nodeType === Node.ELEMENT_NODE || (obj.nodeType === Node.TEXT_NODE && obj.parentElement?.nodeType === Node.ELEMENT_NODE)There was a problem hiding this comment.
Yea, I like that more. See dcbd55a
src/helpers/typeChecks.ts
Outdated
| */ | ||
| export function isFunction(obj: any): obj is (...args: any[]) => any | ||
| { | ||
| if (typeof obj === 'function') return true; |
There was a problem hiding this comment.
When does this check fail? Why are instanceOf and subsequent fallbacks required?
There was a problem hiding this comment.
Good point, I don't think other checks are necessary. See dcbd55a
src/helpers/typeChecks.ts
Outdated
| return obj.nodeName === 'CANVAS' | ||
| && typeof obj.getContext === 'function' | ||
| && typeof obj.toDataURL === 'function'; |
There was a problem hiding this comment.
Should we be exhaustive and ensure that this is first an HTMLElement with isHTMLElement, then further refine to specific element type?
There was a problem hiding this comment.
That's a good idea. See dcbd55a
|
Awesome thanks! |
Yup, I just tested it and it's still working. |
|
The only concern I have with this is this could potentially hide the fact that a project has accidentally imported more than a single version of the PixiJS library. The |
|
Ahh, good point. You technically need to encode the version information, or rely on a uuid that is unique per version in that case as well… |
3 participants
pixi-react does not work inside of a new window using react-new-window. This is something that previously worked in pixi-react 7. The reason it is broken is the usage of the
instanceofoperator. That operator does not work when using multiple JS realms (See MDN). Unfortunately there is no airtight way to check types when using multiple realms. The best you can do is check some properties that are unique to that class.I have tested my changes on the enterprise app I maintain and everything now seems to be working in a new window.