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

feat: skip pending block for already-resolved promises #12274

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Rich-Harris
Copy link
Member

Replaces #11995 — since #11989, this is much easier to do. All we need is to replace queue_micro_task(...) with Promise.resolve().then(...).

I think this makes sense, because there are plenty of reasons why a promise might be resolved on mount — a client-side cache, or a SvelteKit load function that returns a promise that resolves before hydration can occur. If we render the pending block in these cases, the best case is wasted effort; the worst case is visible glitchiness:

But it does mean that mount is arguably not fully synchronous, which is a change. (For clarity: this only concerns mount, the pending block is still rendered when hydrating.) And it means we'll have to update a whole bunch of tests, which I didn't want to do until we have a consensus that this is a sensible direction.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Jul 3, 2024

🦋 Changeset detected

Latest commit: 1f51c1a

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@dummdidumm
Copy link
Member

As articulated in #11995 (comment) I'm still hesitant:

  • SSR is sync, so you can't set a promise into settled state right away. That means before hydration you see the pending state anyway. This removes probably more than half the use cases. I'm also not sure what it means for hydration - is it really that simple to just delay, or does it need additional mismatch logic then ("this was rendered as pending on the server, on the client it's the resolved state, we need to repear the dom")?
  • Messes with expectations: mount is no longer sync in all situations, flushSync no longer flushes synchronously in all situations. This can especially mess with expectations in test contexts

@Rich-Harris
Copy link
Member Author

I'm also not sure what it means for hydration

For hydration nothing changes, it will always start out by hydrating the pending block

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.

None yet

2 participants