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

An effect that inserts iframes may run twice in Safari #2814

Closed
GoodForOneFare opened this issue Nov 4, 2020 · 8 comments
Closed

An effect that inserts iframes may run twice in Safari #2814

GoodForOneFare opened this issue Nov 4, 2020 · 8 comments
Labels

Comments

@GoodForOneFare
Copy link

GoodForOneFare commented Nov 4, 2020

Reproduction

Here's a codesandbox showing a Preact effect running twice in Safari: https://codesandbox.io/s/safari-duplicate-effects-b2lnf.

Chrome result Safari result
safari-duplicate-effects - CodeSandbox 2020-11-04 12-58-32 safari-duplicate-effects - CodeSandbox 2020-11-04 13-00-00

The root cause appears to be Safari running pending promises when an iframe is inserted into the DOM. You can verify by running this code in Safari's dev console:

// Inserting a div results in panda/monkey
defer = Promise.prototype.then.bind(Promise.resolve())
defer(() => console.log("🐵")); document.body.appendChild(document.createElement('div')); console.log("🐼")
🐼
🐵

// Inserting an iframe results in monkey/panda
defer = Promise.prototype.then.bind(Promise.resolve())
defer(() => console.log("🐵")); document.body.appendChild(document.createElement('iframe')); console.log("🐼")
🐵
🐼

As far as I can tell, the exact setup to reproduce is:

  • A parent component that can have its state updated by a child component
  • An effect that updates the parent component's state
  • Another effect that inserts an iframe into the DOM

The state update enqueues components for re-render, then the iframe insertion causes Safari to immediately kick off a re-render inside Preact's current render loop. This happens before the current render has cleared effects arrays, so the effects are executed again.

The sandbox looks contrived, but I ran into this while integrating PayPal into a Preact app.

Steps to reproduce

Running the sandbox in Safari 13.1.2 should illustrate the problem. The same sandbox will show one less element in Chrome.

Expected Behavior

Preact should not run an effect with no dependencies twice.

Actual Behavior

Preact runs an effect with no dependencies twice.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Nov 6, 2020

I can reproduce that in the latest Safari. It seems like it does something different when scheduling requestAnimationFrame callbacks and all useEffect callbacks are called twice. When replaced with a micro-task scheduler it works as expected:

import { options } from "preact";

// Resolves duplicate effect calls in Safari
options.requestAnimationFrame = Promise.prototype.then.bind(Promise.resolve());

@developit
Copy link
Member

Another option would be to use a double promise resolution for the render queue.

@lemonmade
Copy link

lemonmade commented Oct 21, 2021

@developit I was able to resolve this issue in Safari with the following option override:

import {options} from 'preact';
options.requestAnimationFrame = Promise.prototype.then.bind(Promise.resolve().then(() => Promise.resolve()));

Is this what you were suggesting (a double promise resolution)? I assume so but want to make sure I'm not missing something. Would this be something Preact would accept as a contribution? It's a bit of an unusual one since this mechanism is presumably a bit more expensive than the current requestAnimationFrame implementation, and is only needed for an edge case behaviour in one engine.

I also noticed this pattern is internally used to create a function for deferring updates (

? Promise.prototype.then.bind(Promise.resolve())
) — do you know if this could be causing any similar issues?

(Sorry for reviving a zombie issue a year after our last comment, at Shopify we got bit by this issue again :P)

@gaearon
Copy link

gaearon commented Jan 18, 2022

Did anyone file a WebKit bug for this? We're seeing the same issue in facebook/react#22459.

@gaearon
Copy link

gaearon commented Jan 18, 2022

Filed as https://bugs.webkit.org/show_bug.cgi?id=235322.

@developit
Copy link
Member

developit commented Jan 18, 2022

Awesome, thanks for filing that @gaearon! I added a comment over on the trac bug with some bounds around what cases trigger flushing.

@JoviDeCroock
Copy link
Member

Should be fixed in WebKit/WebKit#1909

@mdentremont
Copy link
Contributor

Despite being fixed in Webkit - this issue is still present on Safari <16 which is a huge number of users still.

On the React side, they were able to ship a workaround so that users of the library don't need to implement workarounds, should something similar be considered for preact?

See:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants