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

.ref only supports properties on context #214

Closed
jods4 opened this issue Nov 8, 2015 · 14 comments · Fixed by #328 or aurelia/templating-binding#82
Closed

.ref only supports properties on context #214

jods4 opened this issue Nov 8, 2015 · 14 comments · Fixed by #328 or aurelia/templating-binding#82
Assignees

Comments

@jods4
Copy link
Contributor

jods4 commented Nov 8, 2015

It would be nice to be able to do things like <datatable datatable.ref='section2.datatable'>.

But this currently doesn't work as NameExpression only supports properties directly on context (it does context[property] = value).

As assigning values to path is already supported by Aurelia (e.g. in bindings), I guess it shouldn't be too hard to reuse the same setter code for NameExpression.

It would be the attributes behaviour more consistent and more powerful.

@EisenbergEffect
Copy link
Contributor

Hmm. @jdanyow What do you think of this?

@jdanyow
Copy link
Contributor

jdanyow commented Nov 9, 2015

sounds reasonable. A workaround (after today's release) will be to do something like this:

<datatable with="section2" ref="datatable">

I think this^^^ will work...

Another thing we might want to look into when making changes to ref is the ability to capture multiple references in repeat.for scenarios. I've seen that come up a few times in the gitter. Any ideas for that? Maybe a refs attribute that pushes into an array?

@jods4
Copy link
Contributor Author

jods4 commented Nov 9, 2015

Another thing we might want to look into when making changes to ref is the ability to capture multiple references in repeat.for scenarios.

The idea being that you don't want to add the ref to the for model itself, but a parent (root?) model?

An array seems an obvious idea, but not very friendly to use, as you need to know the index of the model you want the control for. And that index can change over time.

Also, because collections can change over time, it means that the binding may have to regenerate the array several times (by mutation, since coders may have captured the value).

I think that index values might become confusing, e.g. what about nested loops? If I want to store a ref on my root and there are two loops before the refs, what happens?

Other idea: iref (indirect ref?) returning an accessor function, that you can later use like that: f(model) and it returns the reference matching the passed model (or undefined)?

It looks to me like a reversed ko.dataFor()...

In the end, I think as a coder I would just store the ref on the iterated VM. If the model is a business entity, I would probably wrap it into something like itemVM = { entity, ref }. It's the clearest, easiest to use and is doable today.

@EisenbergEffect
Copy link
Contributor

@jdanyow Let's not jump into ref arrays so fast. I want to understand what the developers are trying to do that for first...there may be a better way we want to encourage. I think that arrays of refs could get pretty involved...

@kevmeister68
Copy link

The main problem with a multi-level ref is what you do when the upper-level(s) of the expression are initially undefined? (eg. 'section2.datatable' and section2 is currently undefined). Also, what if "section2" is defined/assigned by a ref itself - you've now got an ordering issue to take care of/resolve.

I know it's a workaround, but it seems that a property setter on the outermost view-model would put the onus on that view-model to disseminate the references elsewhere (ie. into subobjects), for example during bind( ).

Regarding ref arrays, it's not necessarily the same level of "power", but we need to keep in mind that if you are using a repeat-for, then you are already iterating over a container of something (the "data"), so perhaps a simple starting point would be a means to put a "ref" into the "item" (within the container) to store a reference to the view-model for that item.

@jdanyow
Copy link
Contributor

jdanyow commented Nov 12, 2015

Agree- we've run into problems with the auto-creation of object-hierarchies: #205

Lets table this for now- it sounds like there are potentially as many CONS as there are PROS to putting this in.

@jdanyow jdanyow closed this as completed Nov 12, 2015
@jods4
Copy link
Contributor Author

jods4 commented Feb 29, 2016

I was reviewing our codebase and found some hacks that referred to this issue.

I feel a bit like it was closed because the discussion wandered off the subject.

ref support in .for bindings (i.e. ref arrays) is probably hairy but not what I asked for in the topic.

I only want to be able to do custom.ref="section.customVM".

I don't know if the with trick works but it's impractical in my case (due to other bindings being around).

The fact that section may be undefined or not an object is a good point. In my opinion this would seem like a programmer's mistake, so I'm for doing nothing, maybe warn in the console. Reading #205 it seems people like it both ways, so the ref binding should probably do exactly the same thing as other bindings do.

In fact, it is a vexing API that .ref do not accept the same paths as .bind does. There's no reason it shouldn't work exactly the same way and not doing so is breaking coder's expectation.

Going further, if it supported indexing as well (as it should), the .for array question would be solved in a trivial way:

<div repeat.for="item in list">
  <!-- Store on each item: -->
  <thing thing.ref="item.thing"></thing>
  <!-- Store in an array on main VM: -->
  <thing  thing.ref="things[$index]"></thing>
</div>

@EisenbergEffect
Copy link
Contributor

@jdanyow I'd like to revisit this. I saw it come up again in another issue recently as well. I'm wondering if we could just enhance the syntax interpreter to parse the value into an epxression and then pass that to the ref binding which would simply call assign on the expression, passing it the specified reference value type.

What do you think?

@jdanyow jdanyow reopened this Feb 29, 2016
@jdanyow
Copy link
Contributor

jdanyow commented Feb 29, 2016

sounds good to me

@EisenbergEffect
Copy link
Contributor

I'm thinking that would be backwards compatible and add the feature that most people want. I'll assign this to you. (but the IE bug is more important :))

@jods4
Copy link
Contributor Author

jods4 commented Mar 1, 2016

Thanks for the release! Just in time 😄

@snortblt
Copy link

Can you guys clarify what the status is on this? I see the bug is closed, but what exactly was implemented and does this allow capturing of refs within a for loop as mentioned? I could really use this.

@EisenbergEffect
Copy link
Contributor

Oops. Sorry. You can now place binding expressions in the ref :)

@snortblt
Copy link

Thanks! Verified it's working like a champ.

On Mar 11, 2016, at 1:53 PM, Rob Eisenberg [email protected] wrote:

Oops. Sorry. You can now place binding expressions in the ref :)


Reply to this email directly or view it on GitHub.

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.

5 participants