Skip to content

Conversation

Ocean-OS
Copy link
Member

@Ocean-OS Ocean-OS commented Apr 21, 2025

Currently, the compiler assumes that whenever you use array destructuring that the object you're destructuring is an array, and can be interpreted as this:

let [a, b] = object; // svelte thinks this is equivalent: let a = object[0]; let b = object[1];

However, due to iterators, and anything else using Symbol.iterator, this breaks things. This fixes that by reusing the original destructuring structure, like so:

let [a, b] = object; // becomes let [$$1, $$2] = object; let a = $.state($.proxy($$1)); let b = $.state($.proxy($$2));

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
@changeset-bot
Copy link

changeset-bot bot commented Apr 21, 2025

🦋 Changeset detected

Latest commit: aebef7d

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

@Ocean-OS Ocean-OS marked this pull request as ready for review April 21, 2025 23:38
@github-actions
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15813 
@7nik
Copy link
Contributor

7nik commented Apr 22, 2025

This may produce multiple spreadings: example

 // source let [first, second, third] = $state(array); // output let tmp = array, first = $.state($.proxy([...tmp][0])), second = $.state($.proxy([...tmp][1])), third = $.state($.proxy([...tmp][2]));

while it would be better

 let tmp = [...array], first = $.state($.proxy(tmp[0])), second = $.state($.proxy(tmp[1])), third = $.state($.proxy(tmp[2]));

or even

 function to_array(iterable) { return Array.isArray(iterable) ? iterable : [...iterable] } let tmp = to_array(array), ...

though I'm not sure about TypedArrays

@Ocean-OS
Copy link
Member Author

Ocean-OS commented Apr 22, 2025

I'll try the second one, and see how well it works. The only issue is assignments, which don't always result in a temporary variable being made.

@7nik
Copy link
Contributor

7nik commented Apr 23, 2025

Still the same behavior 🤔

@Ocean-OS
Copy link
Member Author

Ocean-OS commented Apr 23, 2025

Just realized that since generator functions are iterators, then this sort of thing (which I use very often) would break.
I'll look into doing something completely different. 🫠

@Ocean-OS Ocean-OS changed the title fix: wrap array destructuring in spread to avoid iterator edge case fix: rewrite destructuring logic to handle iterators Apr 24, 2025
Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

this is a great fix! slight shame we need to reintroduce IIFEs but it's such a rare case that I don't think it matters

@Rich-Harris Rich-Harris merged commit 7826eba into sveltejs:main May 13, 2025
9 checks passed
@github-actions github-actions bot mentioned this pull request May 13, 2025
Rich-Harris added a commit that referenced this pull request May 27, 2025
Rich-Harris added a commit that referenced this pull request May 28, 2025
* revert #15813 * failing test * tweak * tweak * WIP * WIP * WIP * WIP * WIP * working * tweak * changeset * tweak description * Update packages/svelte/src/compiler/utils/ast.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --------- Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants