Skip to content

chore: document and test signals edge case #10228

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

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Conversation

dummdidumm
Copy link
Member

I came across this code and was wondering what it was doing. I saw it was introduced in #9721 but deleted in a #9791 because it seemed it no longer fails, but slightly changing the test still makes it fail as expected. This PR documents the code a bit and adds back the test (in a different form).

That said, I'm not really sure if #9721 should have happened in the first place. It's a bit of code to accomodate for something that reads like a user error - why would anyone first write to something and then read it later on, without it causing an infinite loop? @ogheorghies you opened the related issue - what was the underlying use case for making this work?

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 Jan 19, 2024

⚠️ No Changeset found

Latest commit: 7ecd52e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

What if the infinite loop prevention (including all the current_untracked_writes stuff) only happened in dev?

@dummdidumm
Copy link
Member Author

I'm still having a hard time coming up with a use case where this is actually desirable in practice, i.e. where first writing to something and then reading it doesn't result in an infinite loop.

What would it mean if we only do current_untracked_writes in DEV? For what would we use it? Warn that you've written to something before reading it?

I'm a bit torn on removing infinite loop protection in prod because it could freeze your browser which is worse compared to the app being maybe a bit broken but still functional.

@Rich-Harris
Copy link
Member

It definitely should result in an infinite loop. It's just a question of trade-offs — is it worth it to ship extra code to abort those loops in prod? It seems pretty unlikely that you'd construct an effect (or group of effects) that only fell into an infinite loop in prod (i.e. you'd almost certainly catch it in dev), and either way the app is still broken.

Genuinely asking, I don't have a firm view on this

@trueadm
Copy link
Contributor

trueadm commented Feb 14, 2024

It seems pretty unlikely that you'd construct an effect (or group of effects) that only fell into an infinite loop in prod (i.e. you'd almost certainly catch it in dev), and either way the app is still broken.

You'd think that, but it's just as likely. The amount of times I've seen A/B tests being run that cause prod only errors and no one is seeing them in dev or staging. Haha, the amount of oncall hours spent fighting these fires is definitely reason enough that we keep the errors the same between dev and prod here.

@trueadm trueadm merged commit ba0bdc7 into main Feb 16, 2024
@trueadm trueadm deleted the document-and-test-code branch February 16, 2024 16:41
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.

3 participants