-
Notifications
You must be signed in to change notification settings - Fork 6
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
Feature: Sync-ish Suspense #46
Feature: Sync-ish Suspense #46
Conversation
🦋 Changeset detectedLatest commit: edfe2a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
45aea39
to
3eab732
Compare
type FulfilledThenableState<T> = { status: "fulfilled"; value: T }; | ||
type RejectedThenableState = { status: "rejected"; reason: unknown }; | ||
|
||
type ThenableState<T> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wondering if Promise would be a better name than Thenable
for these variables?
- PendingPromiseState
- FulfilledPromiseState
...
I understand we want to differentiate from standard promises. Though I'm not sure how much the name "thenable" is gaining us here. Took me a second to equate the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH i basically copied React's naming and conventions around this wholesale :D
https://github.com/facebook/react/blob/54f2314e9cfe82baf1c040a55ed4dfff6488f84e/packages/shared/ReactTypes.js/#L126-L148
I guess that it's some sort of precedent, so i thought i'd just stick with that
i suppose you could say that they're calling it a Thenable because they only rely on a subset of the Promise API:
// The subset of a Promise that React APIs rely on. This resolves a value.
// This doesn't require a return value neither from the handler nor the
// then function.
interface ThenableImpl<T> {
then(
onFulfill: (value: T) => mixed,
onReject: (error: mixed) => mixed,
): void | Wakeable;
}
...and in our case it's more like a TrackedPromise
. but i do kinda like Thenable
better, TrackedPromise
is a bit unwieldy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know there's precedence. To be honest, I'm still not sold on the naming, but I don't feel strongly enough about it to block on changes. We can keep this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit on the variable name, but you addressed my suggestions perfectly from the previous PR! Suspexcellent work
oh yeah @bholmesdev do you want a changeset for this, or do you prefer to do that manually before release? (i'm not sure if this is |
Ah, yes I prefer to have changesets on the PR. Do you mind opening one as a |
done it all, i don't have write access so if you're happy with this, go ahead and merge |
This PR is an updated version of bholmesdev/suspense-from-scratch#1
If the contents of a Suspense finish within a deadline (arbitrarily picked 5ms), we don't need to send a fallback, and can send it synchronously. This probably needs some tweaking, but seems like a better UX -- we probably don't want to show a fallback if it's going to immediately get replaced by actual content.
(Similarly, it might be good to have a way to throttle the <script>s that are comming in, so that they don't insert sooner than ~300ms from when the fallback came into view? iirc that's what React's Suspense does to avoid flickering fallbacks)