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 race condition when performing swap for fallback #7945

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Fix race condition when performing swap for fallback #7945

merged 3 commits into from
Aug 4, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 3, 2023

Changes

  • In fallback mode for the router we need to wait for animations (if there are any) and also setTimeout (in case there are none). This causes a race condition that can case swap() to be called twice.
  • Fix is to remove the event listener and the timeout whenever the first wins.

Testing

  • Tested manually. Maybe we should think about adding Firefox as a test runner some how.

Docs

Bug fix

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 2a6a23a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 4, 2023
matthewp
matthewp commented Aug 4, 2023
View reviewed changes
packages/astro/components/ViewTransitions.astro Outdated
// If there are no animations, go ahead and swap on next tick
// This is necessary because we do not know if there are animations.
// The setTimeout is a fallback in case there are none.
setTimeout(() => !isAnimating && swap());
var timeout = setTimeout(() => !isAnimating && fallbackSwap());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a valid use of var!

Copy link
Member

Choose a reason for hiding this comment

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

We could also hoist up a let 😄 var would bleed out of this if block.

Copy link
Member

@ematipico ematipico Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I also think that hoisting a let is more explicit. If it's impossible, we should leave a comment explaining why var needs to be used. E.g.: like TypeScript does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my I totally forgot that a let does hoist too 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok fixed. Let's never speak of this again.

@matthewp matthewp merged commit a00cfb8 into main Aug 4, 2023
@matthewp matthewp deleted the fix-for-real branch August 4, 2023 12:48
@astrobot-houston astrobot-houston mentioned this pull request Aug 4, 2023
Merged
@ethan-ou
Copy link

ethan-ou commented Aug 9, 2023

Thanks @matthewp for this one. I tried using ViewTransitions in 2.10.1 and on Firefox some pages returned a 'null' instead of loading the body of the page. I think this update has resolved it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluwy bluwy bluwy approved these changes

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthewp @ethan-ou @ematipico @bluwy