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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Properly reject promises on server #4464

Merged
merged 2 commits into from Feb 16, 2024
Merged

Properly reject promises on server #4464

merged 2 commits into from Feb 16, 2024

Conversation

ivarnakken
Copy link
Member

Description

Promise.resolve() will not reject upon any of the input promises rejecting, which can cause promises to never stop on the client if the server sends unhandled promises. Simply replacing with a Promise.all() fixes this issue.

Result

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.

No visual changes.

Testing

  • I have thoroughly tested my changes.

Spamming hard refreshes does not put page in limbo 馃憤馃徏

Screen.Recording.2024-02-14.at.18.30.49.mov

@ivarnakken ivarnakken added review-needed Pull requests that need review bug-fix Pull requests that fix a bug labels Feb 14, 2024
@ivarnakken ivarnakken requested review from ollfkaih and a team February 14, 2024 17:32
@ivarnakken ivarnakken self-assigned this Feb 14, 2024
@linear Linear
Copy link

linear bot commented Feb 14, 2024

ABA-819 (Potential SSR) bug on /quotes

ollfkaih

This comment was marked as duplicate.

Sign in to view

@ivarnakken ivarnakken added the approved Pull requests that have been approved label Feb 15, 2024
@ivarnakken
Copy link
Member Author

ivarnakken commented Feb 15, 2024 •
edited

Probably fine, but wouldn't this potentially not run some callbacks if one of them fails?

@ollfkaih Yes you're correct, but I can't think of a better solution

@ivarnakken
Properly reject promises on server
a6b017d 
`Promise.resolve()` will not reject upon any of the input promises
rejecting, which can cause promises to never stop on the client if the
server sends unhandled promises. Simply replacing with a `Promise.all()`
fixes this issue.

Also clean up som effects so that only "valid" promises are made.
@ivarnakken ivarnakken force-pushed the ivarnakken/aba-819-potential-ssr-bug-on-quotes branch from b8b9e8f to a6b017d Compare February 15, 2024 12:02
@ollfkaih
Copy link
Contributor

Probably fine, but wouldn't this potentially not run some callbacks if one of them fails?

@ollfkaih Yes you're correct, but I can't think of a better solution

Allsettled should be the same but ensure all promises are settled?
Obviously only relevant where there are multiple callbacks.

itsisak
itsisak approved these changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@itsisak itsisak left a comment

Choose a reason for hiding this comment

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

馃憤

@ivarnakken
Copy link
Member Author

ivarnakken commented Feb 16, 2024 •
edited

Allsettled should be the same but ensure all promises are settled?
Obviously only relevant where there are multiple callbacks.

@ollfkaih ah yes you're right - this actually just seems like a better options for us here. Replaced them now

@ivarnakken ivarnakken merged commit 62c69d6 into master Feb 16, 2024
4 checks passed
@ivarnakken ivarnakken deleted the ivarnakken/aba-819-potential-ssr-bug-on-quotes branch February 16, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsisak itsisak itsisak approved these changes

@ollfkaih ollfkaih ollfkaih approved these changes

Assignees

@ivarnakken ivarnakken

Labels
approved Pull requests that have been approved bug-fix Pull requests that fix a bug review-needed Pull requests that need review
Projects
None yet
Milestone
No milestone
3 participants
@ivarnakken @ollfkaih @itsisak