Make $capture_state capture local state

[rixo]

Make $capture_state able to capture all writable variables instead of just props.

With this change HMR can use $capture_state to preserve local component state, instead of a custom implementation relying on compiler's output being past down by the bundler.

I made capturing all state the default behaviour because it seems to me that it is consistent with the function's name. However it makes it a breaking change. This might be OK because, as far as I know, this function was intended specifically for HMR and so it probably has no user for now. Otherwise I can default local to false to stay compatible with existing function, if needed.

[Conduitry]

I haven't looked at the implementation in this PR, but it's not clear to me what ought to happen when using export { ... as ... } so the name of the prop is different from the internal state variable. Either we can return only props using their external names - or we can return the entire state using their internal names, which makes it harder to use this hook to look up a prop by its external name. I don't know whether this is a concern.

[rixo]

For HMR, we're not interested in individual props. We just want all props as a whole and, depending on user's preserveState option, all local lets. The payload could be opaque between $capture_state and $inject_state. So prop lookup is not a concern for this purpose. I don't know what other use cases this method can have.

I didn't change the existing implementation for props. It currently uses internal names.

But that's an interesting observation, I think I may have overlooked the export { ... as ... } case a little bit in other places. I'm going to test that it works as intended with the proposed modification.

Also, I thought of another change that could be useful. As an option, $inject_state should filter the props that are passed to it, in order to avoid an "unexpected prop" warning if a prop is removed from a component by a HMR update. Should I add this change to this PR or make another one?

I'm making this PR WIP until I've completed the tests mentioned above.

[rixo]

OK so I finally wrapped my head around this... In fact, it might be better, even for HMR, to be able to capture export_name. But I don't understand Svelte's internals enough to be sure. Your input would be greatly appreciated on this matter. Here's my reasonning...

I think the best strategy to restore a component state when it is replaced on a HMR update is the following:

const cmp = new Cmp({ ...originalOptions, props: updatedProps })
cmp.$inject_state(state)

originalOptions are the options with which the first instance has been created (before other instances may have been created by HMR).

The interesting one is updatedProps. It would be the original props, but with their value updated to the current value in the component. Props that were absent at initial component creation are also skipped in updatedProps, even if they've since somehow been given a value internally in the component.

It would be implemented something like this (if $capture_state would allow to export public names):

const updatedProps = mapObject(
  originalOptions.props,
  export_name => capturedState[export_name]
)

The alternative to this strategy would be to recreate the component with the original options, including original props, and then restore everything with $inject_state.

I have a feeling that the first one may be better because it instantiates the new version of the component directly with the current prop values. But really, I can't be sure. Can there really be a difference? Can this trigger different side effects?

TL;DR Is it possible to have different outcomes when creating a component with different prop values and then, immediately, synchronously calling the same $inject_state on it?

Regarding this issue, the first strategy would need $capture_state to return public prop names. Maybe by adding another flag for this?

// returns only props with their export_name
cmp.$capture_state({ export_props: true })
// returns both props & local variables with their internal names
cmp.$capture_state({ props: true, local: true })

Maybe another method entirely would be cleaner?

cmp.$capture_props()

[Conduitry]

I don't think (even optionally) merging the props and internal state into one variable is a good idea. It's possible that someone could (for whatever reason) have a component where some prop and some non-corresponding internal state had the same name. I think it makes sense to have separate methods for these.

Something else I noticed the other day with the current $inject_state implementation is that it lets you set the values of $-prefixed autosubscription variables, which I don't think it should. $capture_state then probably should also not expose them, but I feel less strongly about that.

[rixo]

I have resolved the conflict with master.

I think it makes sense to have separate methods for these.

I agree. So I'll focus on $capture_state and $inject_state for now.

Something else I noticed the other day with the current $inject_state implementation is that it lets you set the values of $-prefixed autosubscription variables, which I don't think it should. $capture_state then probably should also not expose them, but I feel less strongly about that.

Yes. I have been bitten by this.

This PR filters store subscriptions out both in $inject_state and $capture_state.

I've also added a test that confirms the behavior of $capture_state and $inject_state in regard of props, aliased props, privates, & store subscriptions.

I'm happy with the PR now, so I'm removing the WIP flag.

This is now needed to fix local state support that has been broken by 3.16.0, in a similar fashion as #4041 (the proposed PR #4046 would allow to re-implement the dirty workaround I had in place).

[Conduitry]

Was this left here on purpose? What I had suggested (and what it sounded like you agreed sounded good) was to have separate methods for capturing local state and for capturing props. What I especially don't want is something that returns both at once merged into one object, as that could lead to weird shadowing.

[rixo]

Oops, sorry. I had remained focused on the previous discussion of export { ... as ... }. What I had understood was another method to deal with publicly visible (i.e. aliased) names, not to exclude props from $capture_state entirely.

My current implementation only uses internal names, both in what it returns and what it consumes (like the existing one, in fact). Is there really a risk of shadowing in this case? I don't really appreciate if this state could contain other things that could conflict, now or in the future...

If we had a $capture_props method, we'll need a $inject_props too, right? And this pair will have to deal with aliased vs internal names. Also, we need to sort the names of the methods out because what the existing $capture_state currently does is only exposing props as their internal names.

For now, I'm not sure HMR will ever need public names and, if so, I can workaround it with public API by using compiler's output. svelte-devtools also seems content with current $inject_state. So I was hoping efforts for $capture_props or so could be deferred until it was really needed.

Do you think I should split the methods right now? What should be the naming?

[Conduitry]

Someone could (although it would be crazy) do something like

let foo, bar;
export { foo as bar };

This is why I think props should be entirely excluded from the 'state' methods.

If you think we don't need $capture_props/$inject_props yet for tooling, that's fine, but I don't think $capture_state should have anything to do with props.

[Conduitry]

To be clear, by this I don't mean that $capture_state should remove props from the object it returns, but just that it should disregard any exports and return an object of all of the top-level variables apart from those with injected: true.

[rixo]

OK so, if I understand correctly, you're saying $capture_state should not take any argument and always return every writable variable that is neither injected, nor a store subscription.

That seems to make sense to me so I've updated the PR to do just that, and added a reactive statement in the test case to ensure that they don't end up in the captured state.

[RedHatter]

This is looking mostly good but one small issue. I would like to be able to read $ prefixed variables. Setting not so important but in devtools the ability to see what value is being pulled from the store is very useful.

[rixo]

Updated as per @RedHatter suggestion, since @Conduitry said earlier that exposing store subscriptions in $capture_state was not an absolute no-go. Since now we have a real use case...

Since $inject_state still filters them out, we can still safely pass the raw result of $capture_state directly to $inject_state without having to worry about what's in the payload.

@RedHatter What solution do you have currently for handling aliased prop names (export as)? Isn't this a need for devtools?

[Conduitry]

You can always synchronously get the value of a store, but I suppose I can see why it would be nice if that was exposed directly, rather than making the dev tools do the subscribe-and-unsubscribe.

I'm starting to wonder now though whether it's really best to strip out all injected variables. If you have something like $: foo = f(bar);, might it be useful to be able to see what foo is? You'd be able to anyway if it had been written as let foo; $: foo = f(bar);, which seems like an arbitrary distinction, and I don't really see a reasonable and reliable way to filter out foo in that second case.

And now I'm wondering about how safe it is to just inject arbitrary state at all. If you have let foo = whatever; but nowhere that foo is actually updated, I don't think the compiler will emit code to check whether foo has been changed - so if you update it with $inject_state(), I don't think anything will happen. Is this something to worry about?

[rixo]

You can always synchronously get the value of a store

It could have side effects to do so, couldn't it? I mean, if we just have a storeProp, we can somehow see that it is a store, but if we don't have visibility over $storeProp, we can't be sure that the component has actually subscribed to the store. And if not, the subscribe call could trigger some side effect, which would be unexpected in the context of merely showing the value in the dev tools.

I'm starting to wonder now though whether it's really best to strip out all injected variables.

Yes, indeed. If we can't exclude the semantically equivalent let foo; $: foo = f(bar);, I suppose we have to include both.

if you update it with $inject_state(), I don't think anything will happen. Is this something to worry about?

Not for me. Actually, in the case of HMR, a component could have some props / local variables removed from the component after an update. That means that the component would be injected some extraneous state during normal operation. In this case, it is the intended behavior to ignore it.

In my opinion, what we can do is just expose about everything from $capture_state for inspection, and guarantee that $inject_state will silently ignore any data passed to it that is not actually injectable in the component (i.e. store subs, extraneous variables).

That means $capture_state would finally expose all writable: true variables (my understanding is that non-writable are constant values, and so there's no need to either preserve them in HMR or inspect them in devtools).

Moreover, $inject_state would additionally accept injected: true vars for injection. My understanding is that it is kind of a noop in most cases, since immediately after $inject_state exits, the reactive values would be recomputed from the newly injected values.

For HMR, there might be some weird edge cases where a reactive value computation dependencies change to some state that was not present in the previous version of the component... In this case, the reactive value will have the old value injected and, I guess, won't be recomputed since $inject_state won't trigger invalidations for the newly added variables. Since you said we have no way to detect all such cases... I'd say we can live with this.

@Conduitry Any insight about this? What do you think should be the next step for this PR?

[Conduitry]

Yeah I think now it makes sense for $capture_state() to return all top-level variables, including those injected by the compiler. (So: store autosubscriptions, implicit reactive declaration variables, etc.)

I think it makes sense for $inject_state to ignore store autosubscriptions and any additional mystery variables that don't exist at the top level. There might be cases where it would be nice to get an error or a warning about that, but I see that when this is being used for HMR that's not desirable.

I also suppose that we shouldn't worry too much about the component getting into a weird state because of these methods, because whenever you start mucking around in devtools that's always a distinct possibility.

In short: Yeah sounds good. I think we might be close to the finish line on this finally.

[RedHatter]

What solution do you have currently for handling aliased prop names (export as)? Isn't this a need for devtools?

I use component.$$.props to separate props and map between aliased variables. Something like the following.

let attributes = Object.entries(detail.$$.props).map(([key, value]) => {
  const ret = { key, value: ctx[value], isBound: key in internal.bound })
  delete ctx[value]
  return ret
})

That means $capture_state would finally expose all writable: true variables (my understanding is that non-writable are constant values, and so there's no need to either preserve them in HMR or inspect them in devtools).

I'm not 100% sure about this. Consider the following.

<script>
  export let name
  const initalMessage = `Hello {name}`
</script>
<h1>{initalMessage}</h1>

It might be useful for a user to be able to see the value of initalMessage for reference. e.g.

Props
name: "world"
State
initalMessage: "Hello world"

but maybe not. Honestly could go either way.

Would including non-writable variables be an issue? They would still be filtered out by $inject_state so I shouldn't think so.

[antony]

@rixo I'm probably not the right person for this, but I think this a huge step forward. I'll see if we can get it in front of the right people when they have some available time.

[Conduitry]

Is the intent here (and then again below) to check whether there's a non-noop capture_state/inject_state? Right now, won't this will be truthy whenever we're compiling in dev mode?

[Conduitry]

Hm when I made a change to this effect locally, what ended up happening was that in the debug-no-dependencies test, no instance function was generated at all, and so $$self.$capture_state/$$self.$inject_state weren't set, even to noop. So perhaps what's here now is correct.

[Conduitry]

An alternative would be to make $capture_state and $inject_state actually exist as instance methods on SvelteComponentDev (as noops), and have them be overridden in instance when necessary - which I might actually like better. Thoughts?

[rixo]

Hey, thanks :)

So yeah, it was always true in dev mode (before your change) but I just wanted to write out the real reason, that it was needed for $capture_state and $inject_state.

I agree that having them on SvelteComponentDev is far better. It also explicits these methods' contract by making it clear that they are always present for all components in dev mode. It's great.

[F1LT3R]

I am interested in seeing this land.

Is there any way I can help test this? Do you have steps to test?

[rixo]

@F1LT3R I've passed it through the HMR test suite, and this branch fixes all the test cases for preservation of local state and reactive blocks that are currently failing with master. As far as I can tell, the test suite is otherwise pretty unforgiving, so I'm pretty confident this PR gets us covered regarding HMR needs.

Are you asking out of interest for HMR or dev tools, or are you planning on using this in some tooling of your own?

[Conduitry]

🎉 This has been (finally!) released in 3.19.0. Thank you for your work towards making HMR a reality, and thank you for your patience during the long life of this PR!

[spacedentist]

@Conduitry --- sadly, this commit breaks using Babel macros in Svelte code.

import foobar from "./foobar.macro";

Appearances of the foobar symbol will get mangled by the macro. Typically, at least in my codebase, foobar looks like a function, but the evaluation happens at compile time. Now svelte tries to capture foobar, which breaks the build. There is no "value" for foobar, and after the macro has been evaluated, foobar isn't even defined in the resulting JavaScript.

I'll continue looking into how I can unbreak my code. Any ideas?

[Conduitry]

If I'm understanding you correctly, that's another form of the issue in #4463 and will be fixed by #4475 - does that sound right to you?

[Conduitry]

Hm that's not quite the same situation, and won't be addressed by that PR, because in your case foobar isn't a global - it's an import. @rixo Can you think of a way to also handle this? Could/should we exclude variables that are not referenced or referenced_from_script from the captured state?

[rixo]

I'm afraid it's not the same thing as #4463, foobar is not global in this example.

As I get it, Babel sees something like:

$$self.$capture_state = () => ({ foobar });

and turns it into:

$$self.$capture_state = () => ({ () => { console.log('I am a macro') })

@spacedentist Can you give more details, in particular the error you get and, if possible, the generated code of the component?

Unfortunately, I suppose Svelte would see this variable as referenced at compile time, before Babel intervention (except the user is dull and imports things for no reason, or I'm missing something with macros).

My first intuition is we should not capture imported things, but I don't see how to know that at the place we're generating the code for capture/inject state currently. Is there a way?

[rixo]

If we generate $capture_state like so, it should fix it:

$$self.$capture_state = () => ({ foobar: foobar });

It's more bits, but it's dev code, so I'd be inclined to do this as an easy fix, instead of adding code to Svelte to detect imported variables (except if the code is already there and I missed something?).

[rixo]

@spacedentist I've pushed a branch with the possible fix suggested above in my fork. Can you try it, and see if it does fix your problem?

master...rixo:capture-state-macros-tentative-fix

You can install it in your project with:

npm install -D css-tree@1.0.0-alpha22 rixo/svelte#svelte-v3.19.1-gitpkg

(dunno what's going on with css-tree but you need it too)

I've only fixed $capture_state because, as far as I can tell, your imported macro cannot be seen as writable by Svelte, and so it shouldn't end up in $inject_state (and I hope so, because I've no idea how we could fix it if that was the case).

[tanhauhau]

@spacedentist which babel macro are you using?

It should handle when it is being used in unintended scenarios:

$$self.$capture_state = () => ({ foobar });

is essentially the same as

$$self.$capture_state = () => ({ foobar: foobar });

and i think the macro should either yell at it, or remove the occurrences of foobar at unintended places, resulting

$$self.$capture_state = () => ({ });

[rixo]

Huh? Oh OK, Babel macros are not what I imagined they were apparently.

"breaks using Babel macros in Svelte code" made me think that there was total breakage, and so that the resulting code had to have a parse error. But trying with a random macro, I end up with this code:

        $$self.$capture_state = function () {
          return {
            ms: ms,
          };
        };

This is indeed a runtime error (ms is not defined), but this can really only break HMR and/or dev tools, doesn't it?

So this could be fixed locally by changing $capture_state to:

$$self.$capture_state = () => ({ ms: typeof ms === 'undefined' ? undefined : ms })

This looks more like protecting against a bug that should have been handled beforehand than a proper solution, though.

So the real fix is probably to not capture imported symbols. It also feels in line with not capturing globals to me. But that would require to add a new flag to vars for imported variables...

[rixo]

@spacedentist You should probably open a proper issue with your details, if you want to see this fixed.

[spacedentist]

Hi @rixo, sorry for not replying earlier.

So, looking at your proposed commit 989803d ... that wouldn't help I'm afraid.

So the thing with babel macros is that you use them like this

import foobar from "./foobar.macro";

...which looks like you're importing a symbol from another file, but in fact that's not what happens. Essentially, what Babel does is it loads foobar.macro and executes it at compile time. Within the foobar.macro code, you are basically given all the references in the AST where foobar is used in the JavaScript file that imported the macro. So your macro code can now replace foobar in the AST with whatever code or data you like, and this happens at compile time. So once the macro is done, the foobar symbols won't exist any more in the AST, so the JavaScript output that babel produces won't have foobar in it at all...

So I've written a couple of macros for doing some stuff at compile-time (include constant data, and/or construct complex regexes from a higher-level description). The macro source code checks all AST references of the symbol as which the macro is imported. If the reference constitutes a function call (i.e. foobar(1,2,3)), it looks at the parameters passed in the function call, and then replaces the whole function invocation with some different source code it generates (so e.g. let's say it replaces foobar(1,2,3) with (1+2+3)).

So, the source code Svelte gets to see imports a symbol from foobar.macro and uses it a bunch of time. Svelte used to not touch foobar at all, and so the JavaScript that svelte output would then go into Babel, Babel does all kinds of things including macro magic and things were great.

Now, Svelte generates new code that includes foobar in the definition of that $capture_state object. When Babel is now run, the macro gets to see an additional foobar reference in the AST, and is like "what do you want me to do with this - it's not a function call!". If the macro does not do anything with it, then the eventual output code contains foobar inside the $capture_state definition, but nowhere else - there is not "import" anymore, no other references. foobar just appears there as an otherwise undefined symbol.

How about fixing it? So, it was easy to add a few lines of code to my macros that check if the macro symbol is used as a object property, and if so just replace it with null. Within $capture_state, the macro turns { a, b, c, foobar } into { a, b, c, foobar: null }. This fixed my errors.

So.... what to do? You could say that Svelte does not support that you use Babel macros, so you don't have to care about any of this. That would be a shame, because macros can be super useful. Of course, my modified macro still works, and I'll keep using it, but any future Svelte version might mangle the code in the <script> tags of a Svelte file in whatever way that may break macros again in the future, in a way that may or may not mean that the macro author can fix it (like this time).

One thing that might be helpful would be, if I could declare imports in my Svelte files in a way that tells Svelte not to bother about them. Or if there was a config option to the Svelte compiler to disable this whole $capture_state mechanism. Sure, I won't have HMR, but that's something I can live with.

Also, full disclosure, I am not a frontend/JavaScript expert, so I might not be seeing obvious solutions... or maybe I'm not even making sense...

[rixo]

So, looking at your proposed commit 989803d ... that wouldn't help I'm afraid.

Did you actually tried it?

so e.g. let's say it replaces foobar(1,2,3) with (1+2+3)

This is actually the case I had in mind with my proposed patch.

$$self.$capture_state = () => ({ foobar });
// =>
$$self.$capture_state = () => ({ 6 }); // parse error

// here, the foobar on the left is NOT a reference to foobar in the AST
$$self.$capture_state = () => ({ foobar: foobar });
// =>
$$self.$capture_state = () => ({ foobar: 6 }); // OK

How about fixing it? So, it was easy to add a few lines of code to my macros that check if the macro symbol is used as a object property, and if so just replace it with null. Within $capture_state, the macro turns { a, b, c, foobar } into { a, b, c, foobar: null }. This fixed my errors.

@tanhauhau Seemed to suggest that it was the expected correct behaviour for a macro. So I think it's probably an acceptable "workaround" for user defined macros.

Now, I've proven to myself that there exists other published macros, like the ms one that would be problematic in other ways, by leaving an undefined reference in the code, that would crash at runtime.

Or if there was a config option to the Svelte compiler to disable this whole $capture_state mechanism.

This might be an option actually. Long ago, I had a discussion with Rich H. about HMR, and he mentioned maybe there should be specific options for HMR instead of adding to dev mode.

But this capture / inject mechanism is not used only by HMR. There is also dev tools (and maybe others I don't know about), so I don't know how this option would be called. There is also another, currently not implemented in Svelte, method that HMR would need eventually...

One thing that might be helpful would be, if I could declare imports in my Svelte files in a way that tells Svelte not to bother about them.

We can't add some special syntax just to handle this very edgy case but, all in all, I still think not capturing imports at all would be the right solution to all the problems identified here.

HMR doesn't need them. I don't think they're useful for dev tools. And I can't think of another use case where they'd be useful.

Anyway, once we're able to detect and exclude them, it would be easy to bring them back with a compile option; but, until the need is proven, I think it's better to avoid an extra compile option that would have to be documented, might confuse people, etc.

[rixo]

@RedHatter You don't need to read all of the above, but do you have any opinion about excluding imported references from $capture_state? Like foo in:

import foo from 'whatever'

Would this be cool with dev tools?

[spacedentist]

Thanks, @rixo! Sounds good to me. I'm just figuring these things out myself as we discuss this.

Oh, just one thing about your patch: it doesn't make a difference to me. When my macro changes the code for the "ObjectProperty" the resulting code is changed from ... foobar, ... to ... foobar: null, ... automatically anyway. I'm not sure, but if I had to guess, I'd say that the when Babel parses the code and builds the AST, it turns foobar into foobar: foobar for the AST anyway.

[RedHatter]

@rixo So sorry I took so long to reply. Yes, that would be fine. In fact, I think it would be preferable.