Regression in 3.13: multiple arguments in `use:action`

[ItalyPaleAle]

Starting 3.13.0, Svelte introduced a regression that broke use:action when there are more than 1 argument being passed.

See ItalyPaleAle/svelte-spa-router#59

You can see the REPL here, and open the console to see the result:

• Version 3.14.1 (broken): https://svelte.dev/repl/27728c98515a4f158b65a9e62bd220c2?version=3.14.1 In the console, the first argument is missing and the second one is "undefined"
• Version 3.12.1 behaves correctly: https://svelte.dev/repl/27728c98515a4f158b65a9e62bd220c2?version=3.12.1

[Conduitry]

If this once worked, it was by accident. The docs have never mentioned a syntax for passing multiple arguments. You can use a single array or object to pass multiple things to an action.

I imagine this changed in 3.13 because of the switch to AST-based rather than string-based code generation in the compiler. "hello", "world" is now getting more correctly handled as a sequence expression, which evaluates to the final expression in the sequence. If you look at the generated code, you can see how this is being called: test_action = test.call(null, span, ("hello", "world")) || ({}); As of 3.13, the parentheses are added around ("hello", "world") so it properly becomes one argument.

[ItalyPaleAle]

Thanks for the context. You’re right that it wasn’t a documented behavior and I am not sure why it was working in the first place.

I will update my router, then. I am just a bit disappointed that I’ll have to release a breaking change, just like svelte 3.13 had an (unintentional) breaking change that happened on a minor release.

Given how Svelte is growing and its ecosystem is expanding, might it make sense to start building a “canary in the goldmine” test suite to avoid issues like these in the future? For context: https://github.com/nodejs/citgm

[tanhauhau]

updated the title, you meant 3.13 right?

[ItalyPaleAle]

Yes I’m very sorry. The post said 3.13 but I made a typo in the title

[Conduitry]

I'm not sure having a CITGM suite is something that I'd be ready to embrace. If we'd had one in place, and your library happened to be one of the libraries being used, then - what? We wouldn't have been allowed to fix this bug? We'd have to document it as it's now been forced to be considered a feature?

CITGM suites likely make sense for something like Node - where the user base is so vast, and there are often many layers of abstraction between a user and some library using a potentially undocumented feature/bug, and where planned breaking changes happen every six months - but I don't think I'd want one here.

[ItalyPaleAle]

A CITGM would help developers that are working on the ecosystem as well as end users. The idea isn’t that bugs wouldn’t be fixed, but that we would be able to spot issues downstream more easily and give advanced warning to maintainers so they don’t have to find themselves fighting fires.

In this specific case, I learnt about the breaking change because one user reported the issue on my repo. I expect that others were impacted before but didn’t bother reporting the issue, so there’s been a delay. It took me a day to acknowledge the issue and open this one upstream. I now know I need to make a new release, but I won’t be able to work on this until the weekend (I don’t work as developer in my full-time job and I’m on business travel this week). Of course, this is now a high-priority fire I need to deal with that is impacting my users.

With a CITGM, ideally we would have been notified days (if not weeks) ago. I would have been able to work on a new release at a more comfortable pace, making sure the new code was published before the Svelte update came out. The experience for downstream developers would be much better.

The CITGM can also be used to identify accidental breaking changes and consider wether they should require an update in the major version number as per semver. In this case, the breaking change was on un-document behavior so I’m not sure if it would qualify for a major version update, but there might be other situations where the issue is more real.

Just my .02. Agree Svelte isn’t Node yet, but we are a growing community :)

[pngwn]

I don't think this is really practical.

Firstly, it is down to library authors to ensure that they are using Svelte APIs correctly, undocumented or internal APIs are not public in my view and could change at any moment. If authors choose to use them, then that is on them.

Secondly, you wouldn't have got as much time as you think. We cut new releases relatively often and you might have got a few days notice at best. Hardly much of an improvement.

Our CI test workstream works pretty well and it's almost trivial effort to write, debug, and publish new ones without a whole lot of bootstrapping. I've straightened out some of the tests for bugs that escaped into the wild and plan to do a major sweep in a week or two.

I'd rather put any efforts into expanding what already works, personally.
Also, nothing is stopping library maintainers from cloning the Svelte CI process and running their own tests in addition to ours.