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

"current data object" in templates (re: spread) #1303

Closed
TehShrike opened this issue Apr 2, 2018 · 30 comments · Fixed by #1377
Closed

"current data object" in templates (re: spread) #1303

TehShrike opened this issue Apr 2, 2018 · 30 comments · Fixed by #1377

Comments

@TehShrike
Copy link
Member

Follow-on to #195 -

In order to use the spread functionality to pass arbitrary properties that were applied to the current component on to child components, we need to be able to reference the current data/state object in the template.

I think {{...state}} and {{...data}} and {{...@}} were tossed around, but my vague impression is that {{...this}} is the current top pick.

@Rich-Harris
Copy link
Member

One thing about template tags — {{this}} is already totally valid if you have a property called 'this': https://svelte.technology/repl?version=1.60.0&gist=3a6b97e3ec254df98cc99db546de4520

You can't, however, use it in expressions (even {{(this)}} fails).

So I wonder if the right move here is to make this be an alias for the state object regardless of spread. Would be a breaking change for anyone using {{this}} (though I suspect no-one would be affected by it, realistically). Also, would need to figure out what to do here:

<h2>{{this.thing}}</h2>

{{#each things as thing}}
  <p>What is {{this.thing}}?</p>
{{/each}}

It sort of looks like the second instance would resolve to state.thing, when {{thing}} by itself would equate to state.things[i]. Changing that would require some engineering.

@TehShrike
Copy link
Member Author

I wonder if the right move here is to make this be an alias for the state object regardless of spread

👍 I think this would be the least surprising behavior. I can't think of much reason to use it outside of spread, but that would feel consistent to me.

It sort of looks like the second instance would resolve to state.thing, when {{thing}} by itself would equate to state.things[i]

One of the things that I really really like about Svelte's mustache implementation compared to Ractive is that I never have to think about scopes created by different mustache features.

The only option is to {{#each things as thing}} and then refer to thing, you can't just do .wat or this.wat, you have to be explicit.

I feel pretty strongly that I want Svelte to stay this way, and never create new scopes - devs should be forced to reference thing.wat instead of this.wat inside of that loop.

this should just refer to the component's data object.

For one thing, unless you made this inside of loops inherit objects from the scopes above it (eeeugh), you wouldn't be able to use {{...this}} to pass in arbitrary attributes to child components in a loop.

@TehShrike
Copy link
Member Author

Question unrelated to the previous posts: are we cool with the idea that this would reference only the data object, and not the component - i.e., you wouldn't be able to reference this.method() from the template?

@TehShrike
Copy link
Member Author

(I think having this only reference the data object would be simplest/best, but I worry that some folks might assume it's a reference to the component object, instead of just the data object)

@Rich-Harris
Copy link
Member

are we cool with the idea that this would reference only the data object, and not the component

💯 yes, it would be deeply confusing if it had a dual meaning.

My earlier point about this.thing was purely that if this was just replaced with state, {{this.thing}} would be treated exactly the same way as {{thing}} currently is. In the context of an each block, that means it would refer to the state.things[i] even though it looks like it should refer to state.thing — unless we jump through some unfortunate hoops.

@TehShrike
Copy link
Member Author

oh, I follow you now. Yeah, I would expect it/want it to reference the top-level/component state :-x

Otherwise, I can't use spread in loops, and we're in the I-have-to-think-about-what-scope-I'm-in territory where I don't want to be.

How unfortunate are the hoops? >_<

@jacwright
Copy link
Contributor

jacwright commented Apr 2, 2018

So would using this allow you to label objects in your state events and not have their name collide with the event keyword in listeners? E.g.

<h2>Calendar Events</h2>

{{#each events as event}}
  <div>
    <h4>{{ event.name }}</h4>
    <button on:click="cancelEvent(this.event)">Cancel</button>
  </div>
{{/each}}

@Rich-Harris
Copy link
Member

How unfortunate are the hoops? >_<

Well, #1173 changed the behaviour so that instead of passing down a state object followed by an arbitrary number of parameters ((state, things, thing, i)), plus additional params for every new each or awaiti block), child blocks are passed a 'context' object which has (for example) a thing property. So thing is effectively shadowed, and we can't access the top-level state within the each block. We would have to do something unpleasant like carting around a state.__topLevelState object for those cases.

So would using this allow you to label objects in your state events and not have their name collide with the event keyword in listeners?

well... no. this in an event listener refers to the node, so that you can do this sort of thing:

<input on:focus='this.select()' on:input='updateValue(this.value)' >

@PaulMaly
Copy link
Contributor

PaulMaly commented Apr 5, 2018

Maybe we can unite this feature with one more idea - creating computed properties which depend on all data. We can use rest parameter:

computed: {
    props(...data) => data 
}

And after that spread operator:

<Child {{...props}} />

It'll also allow to pass not all data and filter it somehow:

computed: {
    props(...data) => Object.keys(data).reduce(() => {}, {})
}

Just an idea.

@TehShrike
Copy link
Member Author

@Conduitry
Copy link
Member

I do think that the 'computed properties with a rest param' thing is an interesting idea, but there are some things we need to be careful of. It seems like there are two main ways it could work:

  • The rest param receives no computed properties in its object
  • The rest param receives computed properties that are not themselves based on a rest param value, but not any that are based on a rest param value

The second option is what I was focusing on in a brief Gitter conversation, but I'm now thinking that the first one is better. It's less confusing to understand (try writing docs that don't suck for that second one!), and also it's not any less powerful. If someone wants a rest-param-using-computed-property to depend on the computed properties, they can just list it as an explicit dependency with its own argument. The list of computed properties is known at compile time to Svelte, and will also be known at coding time to the developer, so there doesn't really seem to be any reason to include computed properties in the rest param.

@PaulMaly
Copy link
Contributor

PaulMaly commented Apr 6, 2018

@Conduitry

The list of computed properties is known at compile time to Svelte, and will also be known at coding time to the developer, so there doesn't really seem to be any reason to include computed properties in the rest param.

Completely agree. Perhaps, Store data also should be explicit:

computed: {
     foo(bar, $baz, ...data) {
     },
     bar(qux) {
     }
}

@fjorgemota
Copy link
Contributor

What I understand is that anything we (as a community) define here will probably have two implications:

  • Possible breaking of old code;
  • Dual meaning as in the case of the each loop;

So I will suggest something that may happen to be more "friendly" and customizable to everyone, but it is just an idea, so maybe it is just bad (but to me it makes sense).

Why just do not add a new option, on the object exported by export default, to define the identifier used to identify the global state on the state? I mean, that way a few things would be possible:

  • Let the feature be totally optional (probably disabled by default);
  • Allow use on computed properties (as the user can define the identifier and the compiled would be able to detect the identifier usage on the computed property and work accordingly);
  • Avoid possible conflicts (because the definition would be managed by the user, according to the template, so it's easy to see what is the identifier associated with the global state);
  • Let each cases be more "correctly" managed (because the option would let the user to see that the identifier mentioned is referring to the global state);

As an example, I see something like that happening:


{{#each things as thing}}
  <p>What is {{root.thing}}?</p>
{{/each}}

<script>
export default {
    stateIdentifier: "root"
}
</script>

I do not have an idea actually about the name used to define the option, but I do not see a problem on the approach actually. Of course, let configuration happen that way depends totally on the architecture of the compiled (which I do not know anything about).

Another option would be to define that option as a option to compile, but, after all, it's just ideas.

What do you think? Is that a viable option? (Thanks for creating Svelte, by the way, I absolutely love this project!)

@Rich-Harris Rich-Harris mentioned this issue Apr 14, 2018
@Rich-Harris
Copy link
Member

I made a note in #1338, but it occurs to me that this problem is solved by #1069:

<Widget {...props}/>

<script>
  import Widget from './Widget.html';

  export default {
    components: { Widget },
    computed: {
      props: state => state
    }
  };
</script>

This gives you flexibility over the naming, adds no new syntax, and allows you to exclude props you don't want:

props: state => {
  const { unwanted, alsoUnwanted, ...props } = state;
  return props;
}

@Conduitry
Copy link
Member

How would this handle regular vs computed properties? Computed properties can depend on other computed properties, but things seem they would get hairy if the state object contained computed properties or if other computed properties depend on a computed property that accepts the state object.

@TehShrike
Copy link
Member Author

I assume that the concept of "what is available in the state for computed properties" would stay the same as it is now - containing both raw values, and computed properties.

I write lots of computed properties that depend on other computed properties.

@Rich-Harris
Copy link
Member

At the moment, the compiler uses its knowledge of each computed property's dependencies to sort them topologically:

// `b` will be computed before `c`
computed: {
  c: ({ b }) => b * 2
  b: ({ a }) => a * 2
}

If a computed property accepts the whole state object, it just goes at the end. If you have two or more such properties, they could get out of order if one depends on the output of another, but I think the solution to that is a) document it, and b) preserve the order in which those properties were specified, so it's still deterministic.

@fjorgemota
Copy link
Contributor

well, I like the proposed idea.

With the release of the version 2, what blocks this issue from being implemented with the proposed idea? just to know

@Rich-Harris
Copy link
Member

nothing! someone just needs to make it happen. I'd probably get round to it eventually, but if anyone wants to make a PR... 😀

@jacwright
Copy link
Contributor

jacwright commented Apr 22, 2018 via email

@Conduitry
Copy link
Member

So I'm guessing then that we would not allow a computed property that explicitly depends on a computed property that depends on the whole state object? (That would prevent computed properties that accept the whole state object from all going at the end.)

But I'm still not sure what's a good way to handle passing other (normal or whole-state) computed properties to a whole-state computed property. Would each one simply get the whole state object as it currently exists, whether its current mix of updatedness and nonupdatedness? I'm imagining that would be simplest to implement, but may be confusing to use.

@TehShrike
Copy link
Member Author

If whole-state computed properties get called after everything else, then the only issue would be other whole-state computeds.

There's probably no way to magically resolve how whole-object computed properties see each other - I think as long as they were resolved deterministically in order of declaration it would be fine. Having multiples of this special case computed property would be pretty uncommon I imagine.

@jacwright
Copy link
Contributor

What if we don't do any ordering based on dependencies? Just process them in the order they are defined. This puts all the power into the hands of the developer, as well as make it explicit and clear how it works. Otherwise we are left guessing what might happen in this situation or that situation and may lead to difficult bugs to resolve.

{
  computed: {
    props: state => ({ ...state }), // `props.name` may be stale
    name: ({ firstName, lastName }) => firstName + ' ' + lastName,
    props2: state => ({ ...state }), // `props2.name` will not be stale
  }
}

It is easy to look at the computed property and follow through what happens from top to bottom. Just like event listeners, the first ones are called first, then the second, etc. We can try to "fix" things to help you avoid bugs, but then it makes it more complex and less understandable how it works, possibly causing bugs that you can't understand or easily fix. Though, this is theoretical, I have no practical examples lying around. I do like the simplicity of top to bottom however.

@TehShrike
Copy link
Member Author

There is no way I wouldn't shoot myself in the foot with that behavior :-x

I don't think there's any benefit to ever passing stale values to a single whole-state-computed function.

@jacwright
Copy link
Contributor

Well, there isn't value in stale values, just in a consistent model. I'll backtrack here. I agree with your (@TehShrike) earlier comment about putting those properties last and going in order of declaration.

@Rich-Harris
Copy link
Member

This is available as of 2.3.0: https://svelte.technology/repl?version=2.3.0&gist=f086da6a9c502c6f86abfa8b70eb88ca

(note that Acorn doesn't like object rest syntax, so you can't currently use that in your computed properties)

@Conduitry
Copy link
Member

Acorn doesn't like object rest syntax

Really? What exactly doesn't work? I'm using object spread/rest with (non-Svelte) things that are being bundled by Rollup. Once object spread/rest reached stage 4 of officially being in ES2018, they were implemented in Acorn I believe.

@Rich-Harris
Copy link
Member

See https://astexplorer.net/#/gist/5f225bc72c65b7efe31ea64d57f65f6f/3415116c28fb9f9c8dbe9c2cb457c581294a6dbe

@btakita
Copy link
Contributor

btakita commented Apr 30, 2018

Does this work with store.compute?

@Rich-Harris
Copy link
Member

No. Perhaps we could do that by omitting the second argument:

store.compute('y', state => state.x * 2);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants