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: add infinite loop effect callstack #13231

Merged
merged 3 commits into from
Sep 13, 2024
Merged

feat: add infinite loop effect callstack #13231

merged 3 commits into from
Sep 13, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Sep 13, 2024

This adds a new dev-time only dev_effect_stack variable, which executed effects are pushed to and eventually cleared out after everything's settled. If it doesn't settle however, i.e. you run into an infinite loop, the last ten effects are printed out so you get an idea where the error is coming from. For proper source mapping I also had to add location info to the generated effects.

Closes #13192

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

This adds a new dev-time only `dev_effect_stack` variable, which executed effects are pushed to and eventually cleared out after everything's settled. If it doesn't settle however, i.e. you run into an infinite loop, the last ten effects are printed out so you get an idea where the error is coming from.
For proper source mapping I also had add location info to the generated effects.
Closes #13192
Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: c046928

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

@trueadm
Copy link
Contributor

trueadm commented Sep 13, 2024

Do you have a screenshot what this looks like?

@dummdidumm
Copy link
Member Author

Try for yourself 🙂

@paoloricciuti
Copy link
Member

Love this!

@dummdidumm dummdidumm merged commit a6df4eb into main Sep 13, 2024
9 checks passed
@dummdidumm dummdidumm deleted the effects-stack branch September 13, 2024 13:50
@trueadm
Copy link
Contributor

trueadm commented Sep 13, 2024

We should probably disable this in node, as now I get this when running tests lol:

stderr | packages/svelte/tests/signals/test.ts > signals > schedules rerun when writing to signal before reading it (runes mode)
Last ten effects were:  [
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect]
]

stderr | packages/svelte/tests/signals/test.ts > signals > schedules rerun when writing to signal before reading it (runes mode)
Last ten effects were:  [
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect],
  [Function: $effect]
]

@dummdidumm
Copy link
Member Author

Are these tests triggering the infinite loop guard on purpose? If not, why is this showing up?

@trueadm
Copy link
Contributor

trueadm commented Sep 13, 2024

Just happens when you run all tests?

@RickRyan26
Copy link

5.0.0-next.244 is working great for me, performant and everything, 5.0.0-next.246 is throwing this error so I had to go back to 244. The the last ten effects are printed out but theyre all different normal things so I have no idea what to fix...

@paoloricciuti
Copy link
Member

5.0.0-next.244 is working great for me, performant and everything, 5.0.0-next.246 is throwing this error so I had to go back to 244. The the last ten effects are printed out but theyre all different normal things so I have no idea what to fix...

That's weird...do you have a reproduction?

@RickRyan26
Copy link

RickRyan26 commented Sep 14, 2024

5.0.0-next.244 is working great for me, performant and everything, 5.0.0-next.246 is throwing this error so I had to go back to 244. The the last ten effects are printed out but theyre all different normal things so I have no idea what to fix...

That's weird...do you have a reproduction?

Nevermind, I apologize, 246 works for me now.
I foolishly added reverse() to {#each products.reverse() as product (product.id)} at the same time I updated Svelte and was tricked by the coincidence.

The last 10 effects don't point me to that file tho.

@Rich-Harris
Copy link
Member

Looks like there's an off-by-one error in Firefox:

image

To be honest I'm not sure how much use the transformed function body is in the browsers where it does work — unless I'm well versed in how the compiler transforms code, I'm liable to be confused by this and waste time looking for an occurrence of update(count) in my source code:

image

Feels like we might be better off attaching location information to the effect in development, so that we can do something like this instead:

Uncaught Svelte error: effect_update_depth_exceeded
Maximum update depth exceeded in App.svelte:3:1. This can happen when a reactive block or effect repeatedly sets a new value. Svelte limits the number of nested updates to prevent infinite loops

I'll open an issue

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.

Svelte 5: How to debug effect_update_depth_exceeded in a Svelte 4 library?
5 participants