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

chore: simplify flushing #15348

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

chore: simplify flushing #15348

wants to merge 23 commits into from

Conversation

Rich-Harris
Copy link
Member

I suspected our flushing logic was unnecessarily baroque, and on closer inspection it looks like we can simplify it quite a bit. This PR does so: it gets rid of the scheduler_mode stuff in favour of just flushing the root effect queue until it's empty. This means a) we don't need to mess around with microtasks (theoretically this will improve performance for updates that spawn further updates, though it's unlikely to be measurable) and b) we don't need to do any book-keeping to prevent infinite loops — we just... put everything in a loop.

We can also get rid of the process_deferred middleman, and lose a now-unnecessary try-finally block.

Due to the vagaries of the Animation shim we use in the test suite, this does necessitate a couple of harmless edits to a handful of tests.

I also noticed that the inline docs for both flush_sync and its public counterpart flushSync are out of date or wrong. We don't need the indirection (it was necessary at one point, long ago, when flush_sync took a flush_previous argument, but no longer) so we now just expose the internal function directly. While I was at it I fixed the types — the return value of flushSync now correctly matches the return value of its callback, if provided.

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

Copy link

changeset-bot bot commented Feb 21, 2025

🦋 Changeset detected

Latest commit: 0b18d81

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

@svelte-docs-bot
Copy link

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15348

* Returns void if no callback is provided, otherwise returns the result of calling the callback.
* @template [T=void]
* @param {(() => T) | undefined} [fn]
* @returns {T extends void ? void : T}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just

Suggested change
* @returns {T extends void ? void : T}
* @returns {T}

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, interesting — initially I had just a @template T and it wasn't playing nicely — the return value was unknown when no function was provided:

image

Couldn't figure it out so asked v0, and it gave me some convoluted nonsense with overloads — asked it for a solution without overloads and it gave me this, and it worked. But I now realise that it only worked because of this bit:

- * @template T
+ * @template [T=void]

So I learned two things:

  1. There's JSDoc syntax for declaring the default value of a type parameter, not just its constraints
  2. LLMs are still hopeless

A shame, as I thought I'd found something to add to my sparse-looking 'LLMs actually being useful' list. Guess I'll try again next year.

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.

2 participants