fix: avoid throwing errors if container is no longer in page #424

oscarleonnogales · 2023-04-07T19:10:18Z

Description

We have identified a few errors that zoid throws that cannot be caught by the implementation code (render.catch() and close.catch()) during asynchronous dismounts. Although this can occur in every webpage, it's primarily an issue when using React as seen here: Issue #334

With this change, we first check if the container element has been removed from the DOM or the user has navigated away from the page mid-render. In these scenarios we should avoid throwing errors.

Here are 3 examples:
https://github.com/krakenjs/zoid/blob/main/src/parent/parent.js#L857
https://github.com/krakenjs/zoid/blob/main/src/parent/parent.js#L826
https://github.com/krakenjs/zoid/blob/main/src/parent/parent.js#L1172

These 3 error flows eventually cause the promise to be rejected since the render was never completed, and an error to be displayed on the console: https://github.com/krakenjs/zoid/blob/main/src/parent/parent.js#L732

Reproduction Steps (if applicable)

To reproduce the Window navigated away error, open Chrome developer tools and persist logs. Call paypalButtons.render('container-selector') and then refresh the window or navigate to a different URL before second render is finished.

To reproduce Component closed or Detected container element removed from DOM, Call paypalButtons.render('container-selector') and then immediately remove the container selector before second render is finished.

Why are we making these changes? Include references to any related Jira tasks or GitHub Issues

https://paypal.atlassian.net/browse/LI-7432

wsbrunson

I haven't looked into testing in Zoid so I don't know how reasonable this ask is: can we write a test for this?

src/parent/parent.js

gregjopa · 2023-04-10T14:26:49Z

I haven't looked into testing in Zoid so I don't know how reasonable this ask is: can we write a test for this?

I agree that it would be awesome to add a couple tests to cover this new logic. I'm really excited about this improvement. Great work so far @oscarleonnogales!

src/parent/parent.js

src/constants.js

mdentremont · 2023-04-10T22:43:57Z

This is great, thanks everyone!

codecov-commenter · 2023-04-11T23:26:17Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (31d59fb) 95.63% compared to head (d8f9806) 95.65%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
+ Coverage   95.63%   95.65%   +0.02%     
==========================================
  Files          18       18              
  Lines        1213     1221       +8     
==========================================
+ Hits         1160     1168       +8     
  Misses         53       53

Impacted Files	Coverage Δ
src/constants.js	`100.00% <100.00%> (ø)`
src/parent/parent.js	`95.66% <100.00%> (+0.05%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

gregjopa

This looks great to me! Nice work @oscarleonnogales 💯

…ary test

oscarleonnogales · 2023-04-13T21:33:54Z

test/tests/error.js

-  it("should error out where the domain is an invalid regex", () => {
-    return wrapPromise(({ expect }) => {
-      window.__component__ = () => {
-        return zoid.create({
-          tag: "test-render-domain-invalid-regex",
-          url: "mock://www.child.com/base/test/windows/child/index.htm",
-          domain: /^mock:\/\/www\.meep\.com$/,
-        });
-      };
-
-      const component = window.__component__();
-      return component({
-        onDestroy: expect("onDestroy"),
-      })
-        .render(getBody())
-        .catch(expect("catch"));
-    });
-  });
-


Removing this test because we aren't explicitly checking for valid regex inside of zoid.

The getDomain function already has proper error handling and checking in the cross-domain-utils repository.

spencersablan · 2023-04-14T15:40:42Z

Looks great man 🔥 Love that you moved the error messages into a const ⚡️

wsbrunson · 2023-04-17T18:47:52Z

src/parent/parent.js

+            context === CONTEXT.IFRAME &&
+            isClosed &&
+            ((currentContainer && isElementClosed(currentContainer)) ||
+              isSecondRenderFinished)


where did we land on checking for second render? I thought we said this might cause weird issues but I don't remember if we changed our minds later

We did decide to not use the condition of waiting until second render was finished by itself, as that could cause issues when the container is removed for a legitimate reason before it's finished.

Essentially I combined options 1 and 2 and it seemed to work well. If the container is removed, OR second render is finished then we should call close here.

okay sounds good, thank you!

I am a little worried about adding the new isSecondRenderFinished logic here. The prerender phase still renders inside an <iframe> with no src tag. If that gets destroyed, we would still want this error to be thrown.

src/parent/parent.js

fix: avoid throwing errors if container is no longer in page

f21a25b

wsbrunson reviewed Apr 10, 2023

View reviewed changes

gregjopa reviewed Apr 10, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

gregjopa reviewed Apr 10, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

oscarleonnogales added 2 commits April 10, 2023 17:12

chore: added constants for repeated errors and additional tests

6d807e1

chore: flow issue

9a7a970

oscarleonnogales marked this pull request as ready for review April 10, 2023 22:31

oscarleonnogales requested a review from a team as a code owner April 10, 2023 22:31

gregjopa reviewed Apr 10, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

gregjopa reviewed Apr 10, 2023

View reviewed changes

src/constants.js Outdated Show resolved Hide resolved

gregjopa reviewed Apr 10, 2023

View reviewed changes

src/constants.js Outdated Show resolved Hide resolved

wsbrunson approved these changes Apr 11, 2023

View reviewed changes

chore: renamed error and increased delay

a632f03

gregjopa approved these changes Apr 12, 2023

View reviewed changes

fix: additional check before throwing iframe error and remove unecess…

d8f9806

…ary test

oscarleonnogales commented Apr 13, 2023

View reviewed changes

spencersablan approved these changes Apr 14, 2023

View reviewed changes

wsbrunson reviewed Apr 17, 2023

View reviewed changes

wsbrunson approved these changes Apr 17, 2023

View reviewed changes

gregjopa reviewed Apr 20, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

oscarleonnogales added 2 commits April 20, 2023 12:38

chore: removing additional iframe check

ec3c866

chore: renamed render variable

8e4d461

gregjopa reviewed Apr 20, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

gregjopa reviewed Apr 20, 2023

View reviewed changes

src/parent/parent.js Outdated Show resolved Hide resolved

chore: removed isRendered variable

c7cb4fb

wsbrunson merged commit e2825bb into krakenjs:main Apr 24, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid throwing errors if container is no longer in page #424

fix: avoid throwing errors if container is no longer in page #424

oscarleonnogales commented Apr 7, 2023 •

edited

wsbrunson left a comment

gregjopa commented Apr 10, 2023

mdentremont commented Apr 10, 2023

codecov-commenter commented Apr 11, 2023 •

edited

gregjopa left a comment

oscarleonnogales Apr 13, 2023

spencersablan commented Apr 14, 2023

wsbrunson Apr 17, 2023

oscarleonnogales Apr 17, 2023

wsbrunson Apr 17, 2023

gregjopa Apr 19, 2023

fix: avoid throwing errors if container is no longer in page #424

fix: avoid throwing errors if container is no longer in page #424

Conversation

oscarleonnogales commented Apr 7, 2023 • edited

Description

Reproduction Steps (if applicable)

Why are we making these changes? Include references to any related Jira tasks or GitHub Issues

wsbrunson left a comment

Choose a reason for hiding this comment

gregjopa commented Apr 10, 2023

mdentremont commented Apr 10, 2023

codecov-commenter commented Apr 11, 2023 • edited

Codecov Report

gregjopa left a comment

Choose a reason for hiding this comment

oscarleonnogales Apr 13, 2023

Choose a reason for hiding this comment

spencersablan commented Apr 14, 2023

wsbrunson Apr 17, 2023

Choose a reason for hiding this comment

oscarleonnogales Apr 17, 2023

Choose a reason for hiding this comment

wsbrunson Apr 17, 2023

Choose a reason for hiding this comment

gregjopa Apr 19, 2023

Choose a reason for hiding this comment

oscarleonnogales commented Apr 7, 2023 •

edited

codecov-commenter commented Apr 11, 2023 •

edited