Skip to content
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

Update to version 3.4 throw ex :[Right-hand side of 'instanceof' is not an object] #189

Closed
codesculpture opened this issue Sep 12, 2024 · 8 comments

Comments

@codesculpture
Copy link

codesculpture commented Sep 12, 2024

Duplicating this issue since it is closed, but it should be opened.

This is a interesting issue, since a single change did lot of things from this single commit

Object.hasOwn(value, 'getRootNode') is replaced by 'getRootNode' in value

But which is semantically not equal, as per mdn,

Object.hasOwn

The Object.hasOwn() static method returns true if the specified object has the indicated property as its own property. If the property is inherited, or does not exist, the method returns false.

in operator

The in operator tests if a string or symbol property is present in an object or its prototype chain. If you want to check for only non-inherited properties, use Object.hasOwn() instead.

That being said, if obj has prop and prop is a inherited member,

Object.has(obj, prop) would return false, which is correct
whereas,
'prop' in obj would return true, which is also correct as per spec.

So what happened here is that getRootNode is from Node
But what we have as value is HTMLElement
which inherits Node,
Screenshot 2024-09-12 at 11 54 56 PM

so Object.hasOwn(value, 'getRootNode') probably returns false, since getRootNode is a inherited member from Node in HTMLElement
but 'getRootNode' in value would happily returns true, since that is correct.
So the logic previously returned false but now the same logic returning true, which changed the execution flow and would execute this block of code
https://github.com/1904labs/dom-to-image-more/blob/e7d6ab0535a2477eb1a2b7226105fa1b7c6075a1/src/dom-to-image-more.js#L648C1-L655C10

where global would be a empty object, if dom-to-image-more was loaded through dynamic import using webpack, where webpack is importing dom-to-image-more by eval(import("https://mycdn.com/dom-to-image-more.min.js")) , here if a script is executed through eval the this object would be a empty object {}
For example,
eval(this) would returns {}

so global is {} at this point, so we are doing value instanceof global.ShadowRoot where global is {} thus the rhs of instanceof would be undefined. Throwing error.

The error occured for me while using bundler such as webpack and dynamically imported dom-to-image-more using import().

I believe the fix would be

 function getWindow(node) {
            const ownerDocument = node ? node.ownerDocument : undefined;
            return (
                (ownerDocument ? ownerDocument.defaultView : undefined) ||   
                window || 
                global
            );
        }

i.e the the existence of window would be checked before global (just swapping two lines)

@IDisposable

@IDisposable
Copy link
Member

Thanks for the extensive write-up. Sadly the code you are referencing is not in the current release (neither the hasOwn, nor hasOwnProperty, nor in)

        function isInShadowRoot(value) {
            // not calling the method, getting the method
            if (value === null || value === undefined || value.getRootNode === undefined) return false;
            return isShadowRoot(value.getRootNode());
        }

While I understand that the referenced commit broke your use-case, we still need to have the shadowed elements code work. Since I still don't have a "this doesn't work" reproduction, I can't actually fix this error.

@IDisposable
Copy link
Member

IDisposable commented Sep 12, 2024

I will try swapping those two lines and see if anything breaks, but I don't have any test coverage for use in any browsers besides Chrome and no tests for use in Node or package-managed uses... so it may break things elsewhere.

IDisposable added a commit that referenced this issue Sep 12, 2024
Attempt to fix issue #189
IDisposable added a commit that referenced this issue Sep 12, 2024
Attempt to fix issue #189
@IDisposable IDisposable changed the title [duplicate]: update to version 3.4 throw ex :[Right-hand side of 'instanceof' is not an object] Update to version 3.4 throw ex :[Right-hand side of 'instanceof' is not an object] Sep 12, 2024
@IDisposable
Copy link
Member

There's a prerelease v.3.4.5-beta.0, please try it @codesculpture

@codesculpture
Copy link
Author

Thanks for the extensive write-up. Sadly the code you are referencing is not in the current release (neither the hasOwn, nor hasOwnProperty, nor in)

        function isInShadowRoot(value) {
            // not calling the method, getting the method
            if (value === null || value === undefined || value.getRootNode === undefined) return false;
            return isShadowRoot(value.getRootNode());
        }

This is equivalent to in, in != Object.hasOwn, so this change is also not equivalent to Object.hasOwn

Hence, the logic is changed anyway.

@IDisposable
Copy link
Member

If you can give a PR that adds a test so I can verify the bug and the fix, then I can address the situation. Otherwise, the best I can suggest is using an earlier version. The ShadowDOM fix is not getting backed out for something I cannot reproduce.

Have you tested the 3.4.5-beta.0 release?

@codesculpture
Copy link
Author

codesculpture commented Sep 13, 2024

Seems 3.4.5-beta.0 fixed the issue, this commit

To reproduce this issue,
create a react app using npx create-vite, and choose React, Javascript.
npm i dom-to-image-more@3.4.4

And in App.jsx

const transform = async (dom) => {
  const domtoimage = await import('dom-to-image-more');
  await domtoimage.toSvg(dom, {});
}
function App() {
  return (
    <div> 
    <div id="hello"> </div>
     <button onClick={() => {
      transform(document.getElementById("hello"))
    }}> Hello </button>
    </div>
  )
}
export default App

Click Hello button.

Expected

No errors

Actual
Uncaught (in promise) TypeError: Right-hand side of 'instanceof' is not an object
    at n2 (dom-to-image-more.js:662:20)
    at o2 (dom-to-image-more.js:668:20)
    at Object.isShadowSlotElement (dom-to-image-more.js:709:17)
    at t4 (dom-to-image-more.js:443:26)
    at c2 (dom-to-image-more.js:407:38)
    at dom-to-image-more.js:371:24
    at async transform (App.jsx:3:3)

But if you switch to 3.4.5-beta.0 by
npm i dom-to-image-more@3.4.5-beta.0

Then if you click Hello button,
you no longer see the error.

Seems bundlers (i tested with rollup and webpack), bundles dynamic import by passing custom this object (through .call(obj, ...)) which might not be a window object, then global is what this is. So the fix seems okay which gives high priority for window over global (i.e window || global instead of global || window)

@IDisposable Am not sure is it possible to write test case for this scenario, wdyt

@IDisposable
Copy link
Member

IDisposable commented Sep 13, 2024

Glad that it fixed it... we should just hope it broke nothing new... all my existing tests still work.

Released as v3.4.5 (non-beta)

@codesculpture
Copy link
Author

Glad that it fixed it... we should just hope it broke nothing new... all my existing tests still work.

Released as v3.4.5 (non-beta)

If existing tests works, then i guess we are good. Will try to write test that it imports dom-to-image-more using eval since thats how bundlers were importing.

Also thanks @IDisposable

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

No branches or pull requests

2 participants