fix: fix issue where variable declarations with var or in switch blocks without brackets don't work

[ngtr6788]

Fixes #6706 hopefully

(edit: Some rewording)
One of the comments mentioned in the ticket said that there is an invalid cast from Node to Node[], which is correct. One big reason why there is Not implemented undefined is because we somehow have one variable declaration node wrapped in an array, so it tries to read the AST type of the array which doesn't exist. What I have done instead is that instead of replacing a Node with a Node[], I put the props and inserts to the right of the current node, as siblings in an array and get rid of the current node if necessary, like what the this.replace did but flattened on the same array. If the variable declaration is not a node in an array (ie, a standalone statement), a 'dummy' variable is created for the sole purpose of calling inserts.

Hopefully this does not break things too much. Any feedback, whether on the code, or this comment, is appreciated! 😊

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

• Run the tests with npm test and lint the project with npm run lint

[vercel]

@ngtr6788 is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[dummdidumm]

@Conduitry in #6794 you said you wanted to explicitly disallow usage of vars inside reactive declarations. This PR implements support for it - which way would you like to go?

[ngtr6788]

For the uninitiated, this PR fixes the original bug, and then it goes on to fix another bug not mentioned in the issue, but as a consequence of my first attempted fix (see below), and thus, this post is a bit long, and I try to make sure I get my point across as clearly as possible, so my most sincere apologies. 😞

My first attempted fix is inspired by the implementation in the version prior to the regression in the issue (v3.40.3 IIRC), which assumes that all variable declarations are part of a list of statements, e.g.

{
  var number = writable(3);
  var letter = 'a';
}

and we can simply splice subscriptions and $$props declarations into the array. However, I realized that not all variable declarations are in a list of statements, such as

if (true)
  var x = writable(10);

var y = $x; // add this line and things break

and things break when variables declared with var can be auto-subscribable. This code above will cause a TypeError: parent[key].splice is not a function error, and the stack trace would look something like this:

TypeError: parent[key].splice is not a function
      at Object.enter (compiler.js:40166:20)
      at SyncWalker.visit (compiler.js:778:16)
      at SyncWalker.visit (compiler.js:820:12)
      at SyncWalker.visit (compiler.js:813:19)
      at walk (compiler.js:883:18)
      at Component.rewrite_props (compiler.js:40041:3)
      at dom (compiler.js:35428:13)
      at compile (compiler.js:40837:6)

Therefore, as a preventative measure, I added test cases where variable declarations with var are not part of a list of statements, and check whether parent[key] even is an array or not. When parent[key] is not an array, it can occur in for-loop statements, while statements, do-while statements, etc. It can be weird to reason about, but with some help from ESTree type declarations, I managed to get as many test cases as it can be possible.

You may ask: wait, aren't stores supposed to be declared at the top-level scope in order to be auto-subscribable? Well, stores declared with var (as far as I know) have variable hoisting, so can be declared and used even when they are in many blocks deep, which is why I can even deal with this in the first place.

{
  let store = writable(3); // not allowed
  let storeVal = $store; // not allowed

  var store1 = writable(3); // for some reason, it is allowed
  var storeVal1 = $store1;
}

var storeVal2 = $store1; // this gets picked up
var store2 = store1; // this gets picked up

Let me know if there are anything you want to add, any opinions you may have, any suggestions, any questions, etc. Thank you, and hope you have a wonderful day!

[dummdidumm]

Thank you for the PR (and all your other PRs lately, they are great)! Sadly, we won't be merging this PR for Svelte 4, as with Svelte 5 we might want to drastically change the compiler output and we're not quite sure yet how hard it would be to also support var in that case. Therefore we're playing it safe to not support a (arguably niche) use case which we might have to walk back on later.

Also a heads up that we're now starting the repository restructuring so if you want to contribute best wait until we're done with that (should be sometime next week) and then do contributions against the version-4 branch until it's merged into master again.

[ngtr6788]

That's all right 🙂. However, I was wondering whether to either close this, or just reset this branch to just before I deal with autosubscribable var's and TypeError: parent[key].splice is not a function, that is, just fix the immediate issue? What do you prefer?

[dummdidumm]

I'd prefer to hold off completely from fixing this for now. As I said, we don't yet know how hard it would be to also support this in Svelte 5 so it's best not to fix something which becomes a burden to implement for the new iteration of the compiler.

[ngtr6788]

👍 Well, I guess I'll close this for now. Maybe when Svelte 5 comes around, I might get a go at this again.