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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Apr 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f21a25b
fix: avoid throwing errors if container is no longer in page
oscarleonnogales Apr 7, 2023
6d807e1
chore: added constants for repeated errors and additional tests
oscarleonnogales Apr 10, 2023
9a7a970
chore: flow issue
oscarleonnogales Apr 10, 2023
a632f03
chore: renamed error and increased delay
oscarleonnogales Apr 11, 2023
d8f9806
fix: additional check before throwing iframe error and remove unecess…
oscarleonnogales Apr 13, 2023
ec3c866
chore: removing additional iframe check
oscarleonnogales Apr 20, 2023
8e4d461
chore: renamed render variable
oscarleonnogales Apr 20, 2023
c7cb4fb
chore: removed isRendered variable
oscarleonnogales Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ export const POST_MESSAGE = {
ALLOW_DELEGATE: `${ZOID}_allow_delegate`,
};

export const COMPONENT_ERROR = {
NAVIGATED_AWAY: "Window navigated away",
COMPONENT_DESTROYED: "Component destroyed",
COMPONENT_CLOSED: "Component closed",
WINDOW_CLOSED: "Detected component window close",
POPUP_CLOSE: "Detected popup close",
IFRAME_CLOSE: "Detected iframe close",
};

export const PROP_TYPE = {
STRING: ("string": "string"),
OBJECT: ("object": "object"),
Expand Down
32 changes: 25 additions & 7 deletions src/parent/parent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import {
METHOD,
WINDOW_REFERENCE,
DEFAULT_DIMENSIONS,
COMPONENT_ERROR,
} from "../constants";
import {
getGlobal,
Expand Down Expand Up @@ -316,6 +317,7 @@ export function parentComponent<P, X, C>({
let childComponent: ?ChildExportsType<P>;
let currentChildDomain: ?string;
let currentContainer: HTMLElement | void;
oscarleonnogales marked this conversation as resolved.
Show resolved Hide resolved
let isSecondRenderFinished: boolean = false;
oscarleonnogales marked this conversation as resolved.
Show resolved Hide resolved

const onErrorOverride: ?OnError = overrides.onError;
let getProxyContainerOverride: ?GetProxyContainer =
Expand Down Expand Up @@ -729,7 +731,15 @@ export function parentComponent<P, X, C>({
return clean.all(err);
})
.then(() => {
initPromise.asyncReject(err || new Error("Component destroyed"));
const error = err || new Error(COMPONENT_ERROR.COMPONENT_DESTROYED);
if (
(currentContainer && isElementClosed(currentContainer)) ||
error.message === COMPONENT_ERROR.NAVIGATED_AWAY
) {
initPromise.resolve();
} else {
initPromise.asyncReject(error);
}
});
};

Expand All @@ -749,7 +759,7 @@ export function parentComponent<P, X, C>({
return ZalgoPromise.try(() => {
return event.trigger(EVENT.CLOSE);
}).then(() => {
return destroy(err || new Error(`Component closed`));
return destroy(err || new Error(COMPONENT_ERROR.COMPONENT_CLOSED));
});
});
});
Expand Down Expand Up @@ -823,7 +833,7 @@ export function parentComponent<P, X, C>({
window,
"unload",
once(() => {
destroy(new Error(`Window navigated away`));
destroy(new Error(COMPONENT_ERROR.NAVIGATED_AWAY));
})
);

Expand Down Expand Up @@ -853,8 +863,15 @@ export function parentComponent<P, X, C>({
})
.then((isClosed) => {
if (!cancelled) {
if (isClosed) {
return close(new Error(`Detected ${context} close`));
if (context === CONTEXT.POPUP && isClosed) {
return close(new Error(COMPONENT_ERROR.POPUP_CLOSE));
} else if (
context === CONTEXT.IFRAME &&
isClosed &&
((currentContainer && isElementClosed(currentContainer)) ||
isSecondRenderFinished)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay sounds good, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
return close(new Error(COMPONENT_ERROR.IFRAME_CLOSE));
} else {
return watchForClose(proxyWin, context);
}
Expand All @@ -870,15 +887,15 @@ export function parentComponent<P, X, C>({
.then((isClosed) => {
if (isClosed) {
closed = true;
return close(new Error(`Detected component window close`));
return close(new Error(COMPONENT_ERROR.WINDOW_CLOSED));
}

return ZalgoPromise.delay(200)
.then(() => proxyWin.isClosed())
.then((secondIsClosed) => {
if (secondIsClosed) {
closed = true;
return close(new Error(`Detected component window close`));
return close(new Error(COMPONENT_ERROR.WINDOW_CLOSED));
}
});
})
Expand Down Expand Up @@ -1609,6 +1626,7 @@ export function parentComponent<P, X, C>({
});

const onRenderedPromise = initPromise.then(() => {
isSecondRenderFinished = true;
return event.trigger(EVENT.RENDERED);
});

Expand Down
31 changes: 4 additions & 27 deletions test/tests/error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,9 @@ describe("zoid error cases", () => {
);
});

it("should call onclose when an iframe is closed by someone other than zoid during render", () => {
it("should error out when iframe is closed by someone other than zoid during render", () => {
return wrapPromise(
({ expect, avoid }) => {
({ expect }) => {
window.__component__ = () => {
return zoid.create({
tag: "test-onclose-iframe-closed-during-render",
Expand All @@ -343,17 +343,13 @@ describe("zoid error cases", () => {

onWindowOpen().then(
expect("onWindowOpen", ({ win: openedWindow }) => {
setTimeout(() => {
destroyElement(openedWindow.frameElement);
}, 1);
destroyElement(openedWindow.frameElement);
})
);

const component = window.__component__();
return component({
onClose: expect("onClose"),
onError: avoid("onError"),
onDestroy: expect("onDestroy"),
onError: expect("onError"),
})
.render("body", zoid.CONTEXT.IFRAME)
.catch(expect("catch"));
Expand Down Expand Up @@ -590,25 +586,6 @@ describe("zoid error cases", () => {
});
});

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"));
});
});

Comment on lines -593 to -611
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

it("should error out where an invalid element is passed", () => {
return wrapPromise(({ expect }) => {
window.__component__ = () => {
Expand Down
59 changes: 59 additions & 0 deletions test/tests/remove.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,63 @@ describe("zoid remove cases", () => {
});
});
});

it("should not throw an error when parent container is removed before render completes", () => {
return wrapPromise(() => {
const { container } = getContainer();

window.__component__ = () => {
return zoid.create({
tag: "test-remove-before-render-complete",
url: () => "mock://www.child.com/base/test/windows/child/index.htm",
domain: "mock://www.child.com",
});
};

const component = window.__component__();
const onRenderedPromise = new ZalgoPromise();
const onClosePromise = new ZalgoPromise();

const renderPromise = component({
onRendered: () => onRenderedPromise.resolve(),
onClose: () => onClosePromise.resolve(),
}).render(container);

// manually remove before render completes
container.remove();

return renderPromise.then(() => {
ZalgoPromise.delay(5);
});
});
});

it("should not throw an error when window navigates away before render completes", () => {
return wrapPromise(() => {
const { container } = getContainer();

window.__component__ = () => {
return zoid.create({
tag: "test-navigation-before-render-complete",
url: () => "mock://www.child.com/base/test/windows/child/index.htm",
domain: "mock://www.child.com",
});
};

const component = window.__component__();
const onRenderedPromise = new ZalgoPromise();
const onClosePromise = new ZalgoPromise();

const renderPromise = component({
onRendered: () => onRenderedPromise.resolve(),
onClose: () => onClosePromise.resolve(),
}).render(container);

// manually reload window before render completes
window.dispatchEvent(new Event("unload"));
return renderPromise.then(() => {
ZalgoPromise.delay(5);
});
});
});
});