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

Morph within an animation frame #21

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Conversation

joeldrapper
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@fractaledmind fractaledmind left a comment

Choose a reason for hiding this comment

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

👍🏼

@joeldrapper joeldrapper merged commit c1a4c2d into main Mar 8, 2024
2 checks passed
@joeldrapper joeldrapper deleted the morph-within-animation-frame branch March 8, 2024 23:22
@joeldrapper joeldrapper mentioned this pull request Mar 8, 2024
@midnightmonster
Copy link

I think in general the browser knows when it's executing JavaScript and doesn't need to be in a requestAnimationFrame callback to optimize renders, nor does using requestAnimationFrame guarantee that you or some other code won't do something that requires synchronous layout, which isn't painting as such but is still quite costly, so I'm not sure this particularly helps with performance—but it definitely changes the semantics of the code!

I.e., the work of myMorph.morph is now async with no guaranteed timeline and no explicit way to await its completion, so the actual morphing now happens at an arbitrary-ish time after any other synchronous code that's in execution order after myMorph.morph(). This means:

  1. Previously, some other xhr event handlers could assume that the morph work will be done by the time they run, and now they cannot.
  2. Multiple morphs (e.g. in phlex JS) called one after the other may fail in messy ways, as the DOM node against which a later morph was enqueued may not be part of the DOM anymore by the time it actually runs.

At a minimum, with morph being async, I would expect it to return a promise that's resolved once it's done (which would also simplify your tests).

@midnightmonster
Copy link

Summarizing further comments from Discord, separating the morphing from the identifying the nodes to be morphed introduces infinite race conditions: with this change, literally anything can happen to the page between the time someone calls morph(node, reference) and the time that we attempt to morph node with the new content from reference. It is also not possible for calling code to remove the risk of nodes changing or being queried between call and effect.

Bottom line: with this change reverted, it would still be trivial for calling code to morph within an animation frame callback if desired, but with this change retained, it is impossible to morph synchronously or race-safely.

This is irreconcilably incompatible with (at least) Idiomorph's behavior/API and is not a polite choice for an implement-an-algorithm library to make. The change should be reverted.

@joeldrapper
Copy link
Collaborator Author

Thanks for bringing this up. 🙏

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

Successfully merging this pull request may close these issues.

3 participants