Skip to content

fix: remove readonly validation #10378

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

Closed
wants to merge 2 commits into from
Closed

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Feb 2, 2024

The readonly dev time validation results in false-negative object equality checks: The original object is proxified and thus comparisons could be between the unproxified and proxified version, which will always return false. Fixes #10372

There's also the problem that an object could be passed into a component but then passed upwards into shared state. At that point, it should no longer be readonly, but it's not possible to statically analyze these points. Fixes #10372

Lastly, the each block logic mutates an internal array and that also throws errors with the readonly logic. Fixes #10037 (update: was fixed by #10422 already)

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

The readonly dev time validation results in false-negative object equality checks: The original object is proxified and thus comparisons could be between the unproxified and proxified version, which will always return false. Fixes #10372

There's also the problem that an object could be passed into a component but then passed upwards into shared state. At that point, it should no longer be readonly, but it's not possible to statically analyze these points. Fixes #10372

Lastly, the each block logic mutates an internal array and that also throws errors with the readonly logic. Fixes #10037
Copy link

changeset-bot bot commented Feb 2, 2024

🦋 Changeset detected

Latest commit: 643e4eb

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

@Rich-Harris
Copy link
Member

I'd love to try and find a solution that isn't just removing $.readonly — it provides an important guarantee. The 'can't convert symbol to string' thing sounds like a fixable bug, as does #10037.

The equality thing is trickier, but I'd still like to try and find an alternative. Like, just spitballing — what if at dev time (where $.readonly is used), we replaced a === b with $.is_equal(a, b), which was aware of readonly proxies?

@dummdidumm
Copy link
Member Author

But how can we know where objects that have gotten the $.readonly treatment are used?
The best I came up with is to do some limited static analysis to find the most obvious mutation violations by wrapping prop mutations in some sanity check at dev time. Something like

let { prop } = $props();
// ..
prop.foo = true;
// turns into
$.check_mutation(prop, prop => prop.foo = true)

where check_mutation creates a proxy on the fly just for the duration of that reassignment; or we keep $.readonly but it only adds a symbol property onto the object and $.check_mutation would pick up the corresponding proxy from that and use it for the duration of the mutation.

@Rich-Harris
Copy link
Member

But how can we know where objects that have gotten the $.readonly treatment are used?

We don't, we just transform every === (or !==, or == etc) in dev. The wrinkle is that if you pass stuff into a function in a non-Svelte file things will misbehave - maybe that's fine, maybe it's a dealbreaker

@dummdidumm
Copy link
Member Author

Feels like a deal breaker tbh. I'd rather have some false negatives with the validation compared to super hard to detect bugs that may happen at some point.

@dummdidumm
Copy link
Member Author

dummdidumm commented Feb 6, 2024

Listing the options I see currently:

Remove $.readonly validation completely

Compiler code becomes easier to maintain, no false-positives, but doesn't prevent people from writing spaghetti code anymore

Do isEqual and undoReadonly under the hood

Keep $.readonly as is, but every ==(=)/!=(=) invocation gets transformed to an isEqual invocation (foo === bar -> isEqual(foo, bar)), and every object that is passed to a function outside of the instance scope (imported functions, from module context, indirectly coming from a function which is imported/from module context) is wrapped with undoReadonly which means the original value is passed through instead.
Means more complicated compiler code which differs more significantly between dev and prod, almost no false-positives, false-negatives in case people pass the state to an outside function and mutate in there (but maybe that's also fine/safe depending on the intent / how the state is used).

Add readonly validation to $state directly

There's no separate proxy created from $.readonly, instead the validation is added to $state() directly in DEV mode - in a "sleeping" mode which can be activated somehow. Possible idea is to retrieve the component context on $.readonly invocation (which will still be there, just not creating a proxy anymore) and error if that context is present while mutating.
Means a bit more complicated code but only in the runtime, almost no false-positives, false-negatives in case people invoke the mutation within a forbidden place while the component context isn't set to the one where it would error (example: pass prop to child, pass prop as binding to grandchild. grandchild mutates prop which is disallowed because the binding chain is interrupted between parent and child; but maybe we can handle/detect this case at the prop pass through point)

The readonly problem in general

I've come to the realization that there will probably never be no false positives if we add some form of readonly validation. Let's assume you create some $state() in the parent and pass it down with the intention to never touch that state in the parent again. Basically the ownership is semantically handed off to the child. Because you don't intend to ever do something with it apart from resetting it in certain situations it may feel wrong to use bind: in this instance.
Granted, this is likely an edge case and there are way around this, but I think it's worth thinking about these sorts of use cases to understand how strict the readonly validation should be. In my opinion it's better to have some false negatives compared to having false positives.

@Rich-Harris
Copy link
Member

There is another option, not covered above:

Provide an isEqual function that is aware of readonly proxies

The cases where you need to compare a readonly value with its non-readonly counterpart are probably quite rare. Given that, maybe it's acceptable to require people to do this:

+import { isEqual } from 'svelte';

-if (selected === item) {
+if (isEqual(selected, item)) {

The challenge is getting people to understand when that's necessary, since there's not much help we can provide beyond documentation.

@Rich-Harris
Copy link
Member

closing in favour of #10464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants