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

breaking: overhaul proxies, remove $state.is #12916

Merged
merged 22 commits into from
Aug 20, 2024
Merged

Conversation

Rich-Harris
Copy link
Member

This builds on #12912, with the following changes brought over from #12847:

  • prevents state proxy mutations affecting the original object
  • removes $state.is
  • disallows defineProperty with non-basic descriptors

It also removes the is_frozen check.

To recap: this simplifies the internals a decent amount, gets rid of the weird STATE_SYMBOL property on proxied objects (it's accessible via the proxy, but doesn't exist on the original object), removes the weird abstraction leak around frozen objects, and makes stuff a lot more future-proof.

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 Aug 20, 2024

🦋 Changeset detected

Latest commit: 689d367

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

descriptor.enumerable === false ||
descriptor.writable === false
) {
e.state_descriptors_fixed();
Copy link
Member

Choose a reason for hiding this comment

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

would be good to have a comment here explaining why we have this restriction

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comment - reading up on the invariants, can we relax/change this validation? IIUC we only need to disallow non-configurable properties, because they need to appear as such on the target, too. On the other hand, we cannot allow it at all if the target is no extensible and doesn't contain this property

Copy link
Contributor

Choose a reason for hiding this comment

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

The invariant is correct. We also can’t support non enumerable properties or non-writable because then we need extra overhead to store the extra descriptor info per lookup. See the pr that this came from for context.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough - for future reference, this is where it original came up: #12847 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that, was on mobile earlier so couldn't link it.

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Hello! Question: What is the new way of checking if an object is equal to a reactive object now that $state.is has been removed?

@dummdidumm
Copy link
Member

Either avoid doing that (curious to know why you need it) or use $state.raw instead

@Rich-Harris
Copy link
Member Author

Write your code in such a way that you don't need to compare a proxy with a non-proxy. The original motivation was to solve cases like this...

<script>
  let items = [...];
  let selected = $state(items[0]); // oops, we just proxified `items[0]`
</script>

{#each items as item}
  <!-- this comparison will fail, need to use `$state.is(selected, item)` instead -->
  <div class:selected={selected === item}>...</div>
{/each}

...but since then we introduced $state.raw which is a much better approach:

<script>
  let items = [...];
- let selected = $state(items[0]);
+ let selected = $state.raw(items[0]);
</script>

{#each items as item}
  <div class:selected={selected === item}>...</div>
{/each}

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Hello! I do have this case on a component that renders nav link items in a Bootstrap navbar:

let {
    items,
    activeItem,
} = $props();

Then the active class is set inside the {#each} block by comparing the item in the iterator to the item in activeItem using $state.is. I even bugged the dev to do it like this as opposed to the way it was originally done: With an isActive property inside the item object.

@Rich-Harris
Copy link
Member Author

Right, so it sounds like activeItem needs to use $state.raw

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Indeed, Rich. I'll schedule some time later in the week to do the change and test. Thank you all for the kind explanations. Rich, the docs page still lists $state.is. Unsure if the PR needed to have the docs changed or not.

@Rich-Harris
Copy link
Member Author

ah whoops, good catch — fixed in #12927

@snrmwg
Copy link

snrmwg commented Aug 20, 2024

Right, so it sounds like activeItem needs to use $state.raw

How can I combine $state.raw and $bindable ?

@Rich-Harris
Copy link
Member Author

Can you illustrate why you need to? (Also, if there's an issue here, then please open a new one as it's very easy to lose closed threads)

@snrmwg
Copy link

snrmwg commented Aug 20, 2024

I have implemented a Select component and actually I have exposed the selected item as a bindable property.

@Rich-Harris
Copy link
Member Author

That's not an illustration. Please provide a https://svelte-5-preview.vercel.app repro link showing the problem

@snrmwg
Copy link

snrmwg commented Aug 20, 2024

Reduced demo showing the problem. Not sure if it is an issue or if I am simply using it wrong. The demo is very reduced. My real component allows a property to fetch possible items/options by an async function. It's also possible to provide mappings from options to a value (i.e. to bind just the ID of the selected option).

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Nooo, don't tell me that bindable props are an issue? I have exactly that. I neglected to correctly specify my props before. In reality, I have:

let {
    items,
    activeItem = $bindable(),
} = $props();

This is so the clicking of the nav-link element can inform about the active item, and so the active item can be set externally. Both ways are necessary. What's the problem with $state.raw that prevents comparison?

@snrmwg
Copy link

snrmwg commented Aug 20, 2024

Exteded the demo a bit for comparison with $state.raw

@Rich-Harris
Copy link
Member Author

-let selected = $state();
+let selected = $state.raw();

@webJose
Copy link
Contributor

webJose commented Aug 20, 2024

Indeed, as Rich states, binding to a raw variable makes the comparison works. However, would this suffice long term? For example, I find myself easily forwarding properties from one control to another. For the case of Bootstrap, I have a Dropdown component, which is used as the base of DropdownMenu component, which is used as base to the Autocomplete component.

For the DropdownMenu/Autocomplete relation, both have an items prop. It is not bindable, however, so I guess I'm OK. I'm just trying to illustrate that not always it is possible to exchange $state() with $state.raw(). I suppose the responsible thing would be to test this further in nested bindable scenarios.

@dm-de
Copy link

dm-de commented Sep 1, 2024

I miss "state.is"
To handle multiple overlays, I use a store:
let layers = writable([])

I use new Object to create a uniquie id inside script:
let id = new Object

Now, I do this on mount:
$layers.push(id)

But without state.is - I can not run this code on destroy:
$layers = $layers.filter(i => !$state.is(i, id))
this code do also do not work, because it is a proxy:
$layers = $layers.filter(i => i != id)

Same happen in my keydown event - missing state.is:
if (!$state.is($layers[$layers.length - 1], id)) return
this code do not work, because it is a proxy:
if ($layers[$layers.length - 1] != id) return

@trueadm
Copy link
Contributor

trueadm commented Sep 1, 2024

@dm-de We don't use the object you pass into $state anymore, we just use it as a blueprint for the proxy. This avoids mutating the object entirely.

@7nik
Copy link

7nik commented Sep 1, 2024

@dm-de create stateful ids or use symbols. REPL

@dm-de
Copy link

dm-de commented Sep 1, 2024

Thank you.... Symbol fixes it!

this combination works now:

//in script module:
let layers = writable([])
//in script:
let id = Symbol()
//in onMount:
let len = $layers.push(id)
//in onDestroy
$layers = $layers.filter(i => i != id)
//in keydown event:
if ($layers[$layers.length - 1] != id) return

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.

7 participants