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

Reactive assignments #4

Merged
merged 64 commits into from
Dec 28, 2018
Merged

Reactive assignments #4

merged 64 commits into from
Dec 28, 2018

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 3, 2018

A proposal for simpler, smaller components with a new approach to reactivity.

View formatted RFC

See also the original issue sveltejs/svelte#1826


Due to an unfortunate incident at the commit factory, the original PR for this RFC was merged prematurely. Earlier comments can be read there

@arxpoetica
Copy link
Member

arxpoetica commented Nov 9, 2018

$store.foo says 'store is an object that implements the Store API, and we want to use that API to observe foo'.

I like that! Or maybe just $.foo but that starts looking an awful lot like another familiar API...

Update: to heck with that, jQuery is dead long live the $ store!

@PaulMaly
Copy link

PaulMaly commented Nov 10, 2018

@Rich-Harris, actually, I'm not sure we understand each other correctly. To prevent any misunderstandings, I try to describe controversial points more deeply:

there's basically no situation where you want a component to listen to an event that is emitted by the same component.

I'd argue, for example, I need to make some side-effect when some prop or internal state values are changed. If I we'll be able to use onprops & onupdate callback multiple times, like this:

onprops(() => {
...
});
....

onprops(() => {
...
});

There's not any difference between that and events usage. But events win in this case because we automatically have an opportunity to listen to them outside of the component.

Events are for knowing what's happening with other objects. There's no value in this:

Events are that the source will want that they were. Event producer able to put any payload to the event. So, seems to me, that callback become redundant in the case when we anyway need to have external events corresponding with onprops & onupdate callbacks.

Additionally, having separate lifecycle functions is good for things like tree-shakeability — if on('update', ...) is a possibility you need to include the update lifecycle stuff even if no-one is actually using it.

I'm not sure about it. Seems, the compiler can't be absolutely sure that external event update and props not using somewhere outside of the component.

const comp = new Component({ ... });
...
comp.on('update', () => {
    /* external code wants to know when an internal state changed */
})

So, you need to include this lifecycle stuff anyway. I believe it's not a problem because component needs to have minimal and predictable external api.

In v2, you have to write that export default in a very specific way. You can't, for example, do this:

Actually, this case is really unusual. I don't think that we could to think up many such cases. I'm not sure I will ever want to have a dynamic name of the tag. On the other side really ugly <svelte:meta> solution. Why compiler options should be described in my templates? In html? Btw, for the external observer, it looks more like creating a regular html meta-tags:

<svelte:meta> 
<!-- should become ? -->
<meta ... >

I believe compiler options could be and maybe even should be static.

<script>

/* very clean and habitually */
export default {
  immutable: true,
  tag: 'my-tag'
};
</script>

<!-- really ugly -->
<svelte:meta immutalbe={true} tag="my-tag">

Besides, if we'll ever need to have some static function for configuration or any other purpose, I'll be able to use it with export default in a habitual way:

export default {
  preload() {
     /* hi sapper */
  }
};

I'd argue is sufficiently nicer than <input value={val} on:input={e => val = e.target.value}>

I agree, but my point is - if we'll consider removing component bindings, I think element's bindings are also should be removed, because it have much less meaning than component's bindings.

Definitely a possibility. I kinda like the idea of onprops not having an argument,

I believe this is the most clean solution for spreading props.

The trouble there is that there's no (good) way to make that expression reactive.

Perhaps, we don't need to make that expression reactive? Seems compiler will not be able to make reactive all imports, right?

<script>
  import { some_object, some_variable } from '3rd_party_lib';

  let foo = 0; // 'this is internal state and will be reactive'
</script>
<div>
   <button on:click={e => foo += 1}>Increment</button>

   <!-- how we can make this reactive? -->
   <button on:click={e => some_variable += 1}>Increment</button>
</div>

My point is, in v3 we'll not able to make all imports are reactive, but we'll give an access to any import in our templates. So, maybe we can just use api of these external modules as is?

<script>
  import { some_object, some_variable } from '3rd_party_lib';

  let foo = 0; // 'this is internal state and will be reactive'
</script>
<div>
   <button on:click={e => foo += 1}>Increment</button>

   <!-- we can just use an api of external libs ? -->
   <button on:click={e => some_variable.set(some_variable + 1)}>Increment</button>
</div>

That's not insane, it's called Custom Elements and it's already implemented :)

Not really :) It's more looks like Custom Elements emulation. I'm talking about when customElement: false in compiler.

Right now Svelte component is a constructor which returns an object, but how about to return actual DOM node with extended api? This node can be considered by us as a certain analog of shadowRoot. We can wrap all our component's templates to one additional node, extend that node with Svelte-specific things and return it as imperative version of component for external usage. This approach will gives us full power of DOM API right over svelte component and also will increase compliance with the standard Custom Elements where components has shadowRoot node.

const comp = new Component({ ... }); 

comp.querySelector('.some');
comp.addEventListener('click', () => {});

document.body.appendChild(comp);

You can check how it works in Angular here.

@timhall timhall mentioned this pull request Nov 12, 2018
@timhall timhall mentioned this pull request Dec 7, 2018
@mindrones
Copy link
Member

typo: <input on:keydown"{e => e.which === 39 (missing "=")

export let foo;

beforeUpdate(() => {
// this callback runs whenever props change
Copy link

@timhall timhall Dec 10, 2018

Choose a reason for hiding this comment

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

I believe this also runs when reactive assignments are made, so I think "props" here may be confusing since I understand props to only be those values that are exported.

Suggested change
// this callback runs whenever props change
// this callback runs whenever props or state change


## Unresolved questions

The major TODO is how `Store` should evolve in this new world.
Copy link

Choose a reason for hiding this comment

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

I believe this is resolved now by #5


### Dependency tracking

In Svelte 2, 'computed properties' use compile-time dependency tracking to derive values from state, avoiding recomputation when dependencies haven't changed. These computed properties are 'push-based' rather than 'pull-based', which can result in unnecessary work:
Copy link

Choose a reason for hiding this comment

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

Is this section still valid with the reference implementation of #8? I think #8 is a much cleaner solution, I wonder what would be the expected behavior of function calls in html. I think the most sensible approach would be to re-run the function only when reactive arguments change.

<script>
import { fn } from './functions';
</script>

<ul>
  <li>{fn()} - static, called on initial render</li>
  <li>{fn(a, b)} - dynamic, called whenever a or b change</li>
</ul>

@nolanlawson
Copy link

Thanks for writing this up. I haven't read all the comments, but I read the RFC, and I have a few concerns:

  1. Do we have evidence that the new system will result in smaller/faster code? I see that the method names and property names can be minified now (not that they couldn't before: Whole-app optimisation svelte#1102 (comment)), but other than that I'm not sure if anything will actually get faster from the end-user's perspective as a result of these changes.
  2. Linters will warn on unused variables in this new system. Maybe not a huge deal, but breaking that contract either means I need to carefully apply eslint-disable comments or forgo the benefits of unused variable detection entirely.
  3. In the new system, it seems a lot more difficult for the developer to understand where computed property costs are coming from. In the old system, I could glance at the computed object and get a rough idea of the dependency tree, and see "Aha, this computation is expensive, so I probably want to make sure that the inputs aren't Objects or Arrays so that we don't re-compute unnecessarily." In the new system, using just functions, it seems a lot more difficult to guess what the compiler is doing under the hood because we don't have a straightforward list of inputs/output.
  4. If I want to measure an expensive set() call in Svelte 2 (e.g. using performance.mark() and .measure()), I just wrap the set() call in my mark()s. In this new system, would this work the way I think it would?
let foo
function recompute() {
  performance.mark('start')
  foo = doExpensiveFooCalculation()
  performance.measure('total', 'start')
}

Again, some of this is hard to predict because of the compiler magic.

Overall, I think this proposal is very ambitious and interesting, and it's definitely going to push Svelte in a wild new direction. I'm a little wary of introducing such a huge new change, though, as it will take a long time to upgrade a large app like Pinafore. Also, I for one actually enjoy the data, store, and computed system.

@Conduitry
Copy link
Member

Re: the linting issue, yeah this change does make it less useful to just use e.g. eslint-plugin-html - but I've been working on a Svelte 3 ESLint plugin - https://github.com/Conduitry/eslint-plugin-svelte3 - and I'm pretty happy so far with the experience it's able to provide.

@Rich-Harris
Copy link
Member Author

  1. Do we have evidence that the new system will result in smaller/faster code?

This is tricky to quantify without actually converting an app. The smallest apps will get larger, because there's a bit more in the way of scheduling code etc, but my gut tells me that larger apps will get smaller, for various reasons (more things can be hoisted out of ctx, Svelte can more easily determine which things don't need to have update code generated for them, etc). I'm hoping to try it out this weekend on a couple of apps (just need to update Sapper...) and will report back.

In terms of performance, it's not so much that it'll become faster or slower across the board, but rather that performance will be significantly improved in the worst cases. In Svelte 2 it was much too easy to end up in a situation where you were doing interleaved DOM reads and writes, because of the synchronous updates — this is no longer the case. Longer term we should be able to finesse this further (for example being smart about when transitions are triggered, causing DOM measurements).

  1. Linters will warn on unused variables in this new system

It's true that there'll be some finagling involved depending on your setup. As @Conduitry says this is solvable. In Svelte 2 I always have a problem that isn't solvable AFAICT — referring to built-in-methods inside hooks and custom methods causes red squiggles, because VSCode is smart about figuring out object shapes etc, but (justifiably) not smart enough to know the idiosyncracies of a Svelte export default. So we're effectively exchanging a migraine for a mild headache.

  1. In the new system, it seems a lot more difficult for the developer to understand where computed property costs are coming from

Agree. I think I need to update this RFC with the latest thinking — basically, I came to the conclusion that tracking which values are read during a function invocation isn't going to work, and would result in unpredictable performance characteristics. Instead, we're pursuing an idea that's more similar to Svelte 2's computed in RFC 3. It's a slightly radical idea, but so far it's proven really nice to work with.

  1. If I want to measure an expensive set() call

There are two parts to this — the doExpensiveFooCalculation() and the resulting update. Svelte 3 doesn't transform your code, so the performance.mark/performance.measure will work exactly as you'd expect. It just instruments it with a call to an internal function that invalidates foo and schedules an update (in a microtask) if one isn't currently scheduled.

The update itself is in the framework's control. There are a couple of ways you could profile that — by putting performance.mark in beforeUpdate and performance.measure in afterUpdate, or by manually calling flush (which is currently exported from svelte/internal.js. If it proves necessary, flush could be re-exported from svelte itself).


I definitely recognise that this is going to be a pain to upgrade, and I regret that. I do think it's necessary though — a lot of issues are extremely hard to solve in Svelte 2 but will become easy (or irrelevant) in Svelte 3, and it allows us to jettison a lot of compiler complexity which will let us move faster. There'll be a lot less for newcomers to learn, and components will be easier to write.

Do keep the feedback coming, particularly if there are elements of the 3 RFCs that haven't been explained as well as they could be.

@Conduitry
Copy link
Member

Perhaps we should remove the bit about function call dependency tracking, link to the reactive declaration RFC, and merge this, as it's fairly settled now.

@Rich-Harris
Copy link
Member Author

Agree re removal — have updated it. I think we're supposed to announce a 'final comment period' before merging, if we're respecting the RFC process... I guess we should do that if the three v3 RFCs are in good shape

@Conduitry
Copy link
Member

Actually, before we do that, we should probably do a yak-sweep. There are a few instances of 🐃 remaining, and I'm not sure how many are still relevant.

@Rich-Harris Rich-Harris merged commit 9e5792a into master Dec 28, 2018
@Rich-Harris Rich-Harris deleted the reactive-assignments branch December 28, 2018 18:23
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.