Removing the container element after CLOSE/DESTROY zoid event (after close()), results in uncaught error

[tmilar]

Hi, I've running into this trouble and I couldn't sort it out.

Scenario

• Parent site using React v17.
• User opens an HTML modal, which instantiates and renders a Zoid component (using React driver), as an iframe.
• User Clicks on the background to close it, which internally executes zoidComponent.close() to clean the zoid frame.
• If enough time has passed, so the iframe was already loaded and rendered properly, this results in no errors ✅.
• But if the user opens the modal and quickly closes it, (seemingly, before the zoid iframe finished loading), then the following uncaught error is thrown 🛑.
• This is happening because the container component from the view, is being set for removal right after the EVENTS.CLOSED zoid event.
• What I'd expect: no error thrown when removing the modal container from the view, after the EVENTS.CLOSED (or, EVENTS.DESTROYED) event - or a way to await for the component to properly be disposed, or a way to handle/catch the error.

zoid.frameworks.frame.js:3201 Uncaught Error: Detected container element removed from DOM
    at zoid.frameworks.frame.js:3201
    at anonymous::once (zoid.frameworks.frame.js:1045)
    at elementClosed (zoid.frameworks.frame.js:3165)
    at removeChild (react-dom.development.js:10301)
    at unmountHostComponents (react-dom.development.js:21296)
    at commitDeletion (react-dom.development.js:21347)
    at commitMutationEffects (react-dom.development.js:23407)
    at HTMLUnknownElement.callCallback (react-dom.development.js:3945)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994)
    at invokeGuardedCallback (react-dom.development.js:4056)
    at commitRootImpl (react-dom.development.js:23121)
    at unstable_runWithPriority (scheduler.development.js:646)
    at runWithPriority$1 (react-dom.development.js:11276)
    at commitRoot (react-dom.development.js:22990)
    at performSyncWorkOnRoot (react-dom.development.js:22329)
    at react-dom.development.js:11327
    at unstable_runWithPriority (scheduler.development.js:646)
    at runWithPriority$1 (react-dom.development.js:11276)
    at flushSyncCallbackQueueImpl (react-dom.development.js:11322)
    at flushSyncCallbackQueue (react-dom.development.js:11309)
    at scheduleUpdateOnFiber (react-dom.development.js:21893)
    at dispatchAction (react-dom.development.js:16139)
    at Object.onDestroy (App.tsx:61)
    at zoid.frameworks.frame.js:3320
    at zoid.frameworks.frame.js:2746
    at Function.ZalgoPromise.try (zoid.frameworks.frame.js:387)
    at _loop (zoid.frameworks.frame.js:2745)
    at Object.trigger (zoid.frameworks.frame.js:2749)
    at zoid.frameworks.frame.js:2967
    at Function.ZalgoPromise.try (zoid.frameworks.frame.js:387)
    at destroy (zoid.frameworks.frame.js:2966)
    at zoid.frameworks.frame.js:2983
    at ZalgoPromise._proto.dispatch (zoid.frameworks.frame.js:239)
    at ZalgoPromise._proto.then (zoid.frameworks.frame.js:275)
    at zoid.frameworks.frame.js:2982
    at Function.ZalgoPromise.try (zoid.frameworks.frame.js:387)
    at zoid.frameworks.frame.js:2975
    at anonymous::memoized (zoid.frameworks.frame.js:998)
    at HTMLDivElement.overlay.onclick (pluggy-connect.ts:199)

Context

Below is how the user (parent site) is triggering the "close" action of the iframe which is rendered on the modal. Then, it listens for the zoid CLOSE / DESTROY events, and after that it executes a callback so the container modal can be effectively removed from the view.

      containerTemplate({
        doc,
        dimensions: { height, width },
        close,
        uid,
        frame,
        prerenderFrame,
        event,
        props,
      }) {
// ...
        // zoid component 'close' event handler
        event.on(EVENT.CLOSE, () => {
          document.body.classList.remove(modalVisibleClassName);
          const { onClose } = props;
          // callback for the parent site so it can update the view
          if (onClose) {
            onClose();
          }
        });

        // zoid component 'destroy' event handler
        event.on(EVENT.DESTROY, () => {
          const { onDestroy } = props;
          // callback for the parent site so it can update the view
          if (onDestroy) {
            onDestroy();
          }
        });

          // overlay modal close handler 
          overlay.onclick = () => {
            close()
          };
// ...

This is how the parent app is instantiating the component:

export const MyReactZoidComponent = ({
  tag,
  url,
  ...props
}) => {
  const Zoid = useMemo(() => {
    zoid.destroy()
    return zoid.create({ tag, url }).driver("react", { React, ReactDOM })
  }, [tag, url])

  return Zoid ? <Zoid {...props} /> : null
}

And this is how the parent site renders it:

function App() {
  const [isModalOpened, setIsModalOpened] = useState(false);

  const myZoidComponentProps: MyZoidComponentProps = {
    url: REACT_APP_CONNECT_URL,
    connectToken: REACT_APP_API_KEY,
    onError: (error) => {
      console.error('Whoops! Error result... ', error);
    },
    onSuccess: (data) => {
      console.log('Yay! Success!', data);
    },
    onOpen: () => {
      console.log('Modal opened.');
    },
    onClose: () => {
      console.log('Pluggy Connect modal closed.');
      // setIsModalOpened(false);
    },
    onDestroy: () => {
      console.log('Pluggy Connect modal destroyed.');
      try {
        setIsModalOpened(false);
      } catch (error) {
        // NOTE: this catch isn't working, even though the line above shows up in the stack-trace
        console.error('whoops..', error);
      }
    },
  };

  const openModal = () => {
    setIsModalOpened(true);
  };

  return (
    <div className="App">
      <button onClick={openModal}>Open Modal</button>
      {isModalOpened && <MyReactZoidComponent {...myZoidComponentProps} />}
    </div>
  );
}

What I've tried

1. Adding a try {} catch {} block arround the close() method, and also a promise .catch(). None have worked.

          overlay.onclick = () => {
            try {
              close().catch((error: unknown) =>
                console.warn('Modal close handler resulted in an error ', error)
              );
            } catch(error) {
              console.warn('Caught! Modal close handler resulted in an error ', error)
            }
          };

2. Surrounding the <Zoid/> react component with an error boundary. Didn't work.

**What I'd expect **

• The zoid component close() call should not result in an unhandled error, or
• The error should be able to be caught/handled using .catch() or try {} catch {} in some place, or...
• A working way to properly await for the .close() / EVENTS.CLOSE action to be fully completed to let me safely remove the container component from the page.

[tmilar]

I've found out about this overrides.onError (internal?) method. But seems to be no way of using that from userland..

on src/parent/parent.js:

export function parentComponent<P>(options : NormalizedComponentOptionsType<P>, overrides? : ParentDelegateOverrides<P> = getDefaultOverrides(), parentWin : CrossDomainWindowType = window) : ParentComponent<P> {
   // ...

   /* onErrorOverride, but seemingly not available for config */
    const onErrorOverride : ?OnError = overrides.onError;
  // ...

   
   /* the actual onError handler */
    const onError = (err : mixed) : ZalgoPromise<void> => {
        if (onErrorOverride) {
            return onErrorOverride(err);
        }

        return ZalgoPromise.try(() => {
            if (handledErrors.indexOf(err) !== -1) {
                return;
            }

            handledErrors.push(err);
            initPromise.asyncReject(err);

            return event.trigger(EVENT.ERROR, err);
        });
    };

   // ...  

    /* the render method. Note that this returns a "ZalgoPromise". Also note that this *asynchronously* handles errors in `.catch(err)`, and afterwards has a `.then()` that throws the error */
    const render = (target : CrossDomainWindowType, container : string | HTMLElement, context : $Values<typeof CONTEXT>) : ZalgoPromise<void> => {
        return ZalgoPromise.try(() => {
            //...
            return ZalgoPromise.hash({
                initPromise, buildUrlPromise, onRenderPromise, getProxyContainerPromise, openFramePromise, openPrerenderFramePromise, renderContainerPromise, openPromise,
                openPrerenderPromise, setStatePromise, prerenderPromise, loadUrlPromise, buildWindowNamePromise, setWindowNamePromise, watchForClosePromise, onDisplayPromise,
                openBridgePromise, runTimeoutPromise, onRenderedPromise, delegatePromise, watchForUnloadPromise
            });
            
        }).catch(err => {
            return ZalgoPromise.all([
                onError(err),
                destroy(err) 
            ]).then(() => {
                throw err; ///  (!) here is the throw of the uncaught error !!!
            }, () => {
                throw err;
            });
        }).then(noop);

Now, when using 'zoidComponent.render()', it's possible to catch errors by adding a .catch() like so:

const target = document.querySelector("body")
zoidComponentInstance.render(target).catch(err => console.error("render error..", err)

But, for the react driver, it seems to not be possible to do that.
See the driver logic below.

on src/drivers/react.js

export const react : ComponentDriverType<*, ReactLibraryType, typeof ReactClassType> = {

    register: (tag, propsDef, init, { React, ReactDOM }) => {

        // $FlowFixMe
        return class extends React.Component {
            render() : ReactElementType {
                return React.createElement('div', null);
            }

            componentDidMount() {
                // $FlowFixMe
                const el = ReactDOM.findDOMNode(this);
                const parent = init(extend({}, this.props));
                parent.render(el, CONTEXT.IFRAME); /// !! as seen above this is actually *async* 
                this.setState({ parent });
            }

            componentDidUpdate() {

                if (this.state && this.state.parent) {
                    this.state.parent.updateProps(extend({}, this.props)).catch(noop);
                }
            }
        };
    }
};

So, the issue seems to be either that:

• it's not possible to override the onError zoid component handler from userland
• the react driver has a leakage on the parent.render(el) line, it's executing a promise, but it's not allowing for it to be awaited, adding a .then(), or a .catch().

[mnicpt]

@tmilar Thank you for bringing this up. We have also run into this with our Smart Payment Buttons and needed to give time for the container to be rendered as you have mentioned above. I would recommend setting a delay before calling onClose like the following:

event.on(EVENT.CLOSE, () => {
          document.body.classList.remove(modalVisibleClassName);
          const { onClose } = props;
          // callback for the parent site so it can update the view
          if (onClose) {
            ZalgoPromise.delay(1000).then(() => {
              onClose();
            });
          }
        });

Please close if this works for you.

[tmilar]

Hi @mnicpt , thanks for your quick response.
The workaround of explicitly waiting for a timer before actually calling the onClose callback, might work...
But the problem I'm seeing with that is that it wouldn't feel like a good UX for the user to have to wait an extra arbitrary timer each time it wants to close the modal.

It'd be preferable to only wait the actually needed time. For example if it fully loaded, then close it immediately. And if it didn't finish loading, actually wait the right time until it's done.

Also, there is the possiblity that an explicit timer might not be enough in some scenarios (ie. slow internet connection) and still throw the error.

Now I have 2 questions.

1. If I somehow managed to catch the error, would just logging and dismissing it cause any problems on the app? ie. corrupted state?
2. Can you confirm if the overrides.onError I mentioned above is somehow available to be configured by the app to prevent this behavior?

I'm also thinking that an improvement for the react driver could take place, by handling the parent.render() promise result and letting it bubble up in the react stack somehow. For example like this: https://medium.com/trabe/catching-asynchronous-errors-in-react-using-error-boundaries-5e8a5fd7b971

[gregjopa]

Hi @tmilar, I was reviewing your implementation and was wondering if you could solely rely on the zoid close() function for cleaning up the DOM instead of having the React component do it. Ex:

export const MyReactZoidComponent = ({
  tag,
  url,
  ...props
}) => {
  const Zoid = useMemo(() => {
    zoid.destroy()
    return zoid.create({ tag, url }).driver("react", { React, ReactDOM })
  }, [tag, url])

  // return Zoid ? <Zoid {...props} /> : null
  // always return the zoid component here no matter what. Let the close() function be responsible for DOM cleanup.
  return <Zoid {...props} />
}

The zoid close() function should be responsible for removing the DOM of the zoid component. There are some MutationObservers being used with zoid and and calling close() cleans everything up properly. If you destroy it yourself by having React rip the component out of the DOM then you will end up seeing the error "Uncaught Error: Detected container element removed from DOM".

One other thing to note is the close() function is async. You'll want ensure that is done before having React remove it (ex: return Zoid ? <Zoid {...props} /> : null).

          // overlay modal close handler 
          overlay.onclick = () => {
            close().then(() => {
              console.log('closing successful!');
            });
          };

[bluepnume]

I really like the idea of using React error boundaries. If anyone wants to take that up, that would be awesome.

I'm also debating, should we just make close() resolve the initial render promise, if it hasn't already been resolved? The initial thinking was, if the asynchronous render does not succeed for ANY reason, reject the promise returned by render (which causes an error in the console if it's not caught). But perhaps an explicit render() then close() is a legitimate case for just resolving the promise and treating it as a happy case, since it's legitimate to close a component immediately after rendering it.

The only concern is, we would be breaking the strong guarantee that render().then(() => { ... }) means the component definitely rendered.

[tmilar]

Hi @gregjopa, thanks for your reply.

Regarding this:

I was wondering if you could solely rely on the zoid close() function for cleaning up the DOM instead of having the React component do it.

It could be possible, but I'd prefer my shared component to have as less limitations as possible. I'd like to allow the option to a remote site of which I might not know anything about, to just remove the component from the view and not have any uncaught errors.
I've read even of some frameworks / tools that are quite aggressive by removing/replacing entire elements from the DOM so it would not be nice to have unexpected errors just due to this, if we can prevent it.

One other thing to note is the close() function is async. You'll want ensure that is done before having React remove it (ex: return Zoid ? <Zoid {...props} /> : null).

Yes, I've noted it, and attempted to handle it that way.
But waiting for the close() async result is not a guarantee of not having any errors, since the exception is actually thrown by the render() method, which is bound to the react render cycle.
Also, since the error is being thrown asynchronously, and it's not being handled by the react driver, there is no possibility of preventing the uncaught error.

[tmilar]

@bluepnume, thanks for chiming in.

I think a fair solution would be having the render() async result be included in the react render cycle, and have it fail in a more manageable way.
This would result in giving options of handling the error, such as using an ErrorBoundary around the zoid component to manage it.

Now, regarding this:

I'm also debating, should we just make close() resolve the initial render promise, if it hasn't already been resolved? The initial thinking was, if the asynchronous render does not succeed for ANY reason, reject the promise returned by render (which causes an error in the console if it's not caught). But perhaps an explicit render() then close() is a legitimate case for just resolving the promise and treating it as a happy case, since it's legitimate to close a component immediately after rendering it.

The only concern is, we would be breaking the strong guarantee that render().then(() => { ... }) means the component definitely rendered.

-> I'm not very fond of the implementation details. Inspecting the logic I couldn't understand why there has to be an exception on the first place if the user decided to remove/close the component quickly enough.

Yes, it's clear that there was an expectation from the zoidComponent to load/render an iframe - but a failure to do that, caused by user deciding to cancel/close/abort it, should not result in an unexpected error I think.

My instinct would say to not change/break the API anyway, just would make sure that all possible success/error paths are predictable, and manageable by any user that intends to do so.

[bluepnume]

I think a fair solution would be having the render() async result be included in the react render cycle, and have it fail in a more manageable way.

I think that's a fair ask.

I couldn't understand why there has to be an exception on the first place if the user decided to remove/close the component quickly enough.

So: when you call render() for a component, because rendering is asynchronous, zoid returns a promise. We have to make a decision at some point, regardless of what happened, do we:

a) Resolve the promise
b) Reject the promise (which, if unhandled, results in an error creeping into the console)
c) Leave the promise unresolved

This is normally a simple decision. If the render succeeds -> resolve the promise. In any other case -> reject the promise. This way, render().then(...) gives a strong guarantee that yes, the component was rendered, and nothing prevented it from rendering. There are no cases where we leave the promise unresolved.

So to answer your question:

why there has to be an exception on the first place

It's because anything other than a resolved render promise, is a rejected render promise. And any unhandled, rejected promise, ends up getting displayed as a red error message in the console.

just would make sure that all possible success/error paths are predictable, and manageable by any user that intends to do so.

So yeah, giving a way for the React driver, and the user of the react driver, to handle errors during render, definitely sounds like a good idea, no matter which way we go with close(). There are many things that can go wrong during a render, and they should all be handleable regardless of which driver you're using.

(Actually, if it's the case that onError is not called in this case, that sounds weird/problematic to me, and we should probably fix that. Need to dig in there.)

My instinct would say to not change/break the API anyway

This would be my instinct too, normally.

But I am concerned about this "close during render" case. At PayPal we see this happening a lot -- it's very easy for DOM elements to be removed for a lot of reasons, and a lot of clients who consume zoid components don't handle rejected render() promises, which does result in a lot of console error noise.

Maybe we just accept that, so long as there is technically a way for clients to handle those errors. Maybe we make "close during render" a non-rejecting case. Maybe we reject the render promise, but make it some kind of "silent reject" where it doesn't get propagated to the console if it happens. Need to think through the options...

[tmilar]

Hi @bluepnume , I've just submitted PR #335 on an attempt to improve this.

On my project I've created a local version of the react driver to keep moving, so I've kind of tested it, and seems to be working as expected now: no uncaught errors after component close() or destroy().

[mnicpt]

@tmilar Looks like you have a flow error. Any update on when you may have tests for this?

[badeAdebayo]

any update on this issue? @bluepnume i see this error every time i click the back button before the iframe finishes loading

[edwinfinch]

Any update on this?

[naeem-gitonga]

February 2024, Any update on this?

[thanatos11]

still having issues with this.... any update?

[ensonal]

On March, are there any update?

[DavidBrilliantFL]

We're also experiencing this issue over here and could use this fix.