Support multi-dimensional refs

[Arcath]

Allows refs to be multi-dimensional

<input type="checkbox" ref="selection[foo]" />
<input type="checkbox" ref="selection[bar]" />

can be accessed with component.refs.selection.foo and component.refs.selection.bar.

Can go as deep as you want.

[lierdakil]

This looks counterintuitive. I would expect divs[first][second], not this.

[Arcath]

Now you mention it it does look better your way. Will update the pr when I get chance.

…

On 3 Apr 2017 8:28 p.m., "Nikolay Yakimov" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/unit/initialize.test.js <#50 (comment)>: > + update () {} + } + + etch.initialize(component) + + expect('divs[first]' in component.refs).to.be.false + expect('divs' in component.refs).to.be.true + expect(component.refs.divs).to.have.all.keys(['first', 'second']) + expect(component.refs.divs.second.textContent).to.equal('second') + + }) + + it('allows for deep multi dimensional refs', async function () { + let component = { + render () { + return <div ref='divs[test[first]]'>hello</div> This looks counterintuitive. I would expect divs[first][second], not this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#50 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABMmVgxO1M01sgMHPO1SGhPbuqdsJ7nks5rsUhLgaJpZM4Mx9s2> .

[Arcath]

As suggested sub keys are now defined as one[two][three][four]

[Arcath]

I'm working on maintaining backwards compatibility at the moment.

It needs a flag to enable this new feature (then next release inverted to disable it).

This is so that refs matching foo[bar] will still work as expected after an update.

[nathansobo]

This is a cool idea that I considered as well in the past, but I'm somewhat reluctant to bring on this extra complexity. I want to keep Etch as simple and focused as possible...

@maxbrunsfeld, @as-cii Do you have any thoughts on this? I do see the potential value of this feature.

Also, if we were to decide to add this, one thought is that it might make more sense to use a totally different prop name rather than the flatRefs flag on the component. Maybe something like arrayRef or objectRef pointing to an array of the ref name and key so we don't have to do work processing the string, something like this...

$.div({arrayRef: ['childNodes', 0]})

[Arcath]

@nathansobo flatRefs didn't sit well with me but I was unsure what a better name for it would be.

All names etc... can be changed to better conform with standards, I was focusing on getting it working first.

Using arrayRef and objectRef is probably a better way to go, aside from the saving on regex and string parsing it will remove the need for the users to put variables in strings etc...

//My way
{ref: 'something[' + something.id + '][name]' }

//arrayRef
{arrayRef: ['something', something.id, 'name]}

[Arcath]

I've updated the PR to use arrays in the ref option to use a multi-dimensional ref.

{ref: ['foo', 'bar']}

[Arcath]

@nathansobo I'm pretty happy with how this works now.

I didn't split the functionality into another property for 2 reasons.

1. It would require passing another option through the various functions and logic to decide if it should be multi-dimensional or not. By casting a string into te first element of an array the functions work the same either way and should be easier to maintain.
2. This removes any potential issues with users defining both ref and arrayRef.

Let me know if there is anything that needs adding/correcting.

[nathansobo]

Forgive me if I'm missing it, but do you have a test for the removal case. I'm specifically concerned about the logic for removing sub-object when all refs at a given level of nesting disappear.

[Arcath]

You're right. Written a test for moving components around within refs and there were a couple of bugs with how buildRefs.clear works and with my edits to updateRefs.

I've fixed them now and added it to this PR.

[Arcath]

@nathansobo any news on this PR?

[nathansobo]

I've been on the fence about adding the complexity. @maxbrunsfeld can you help me think about whether we should merge this. I'm leaning yes.

[larkin]

@nathansobo ISTM that if you need nested refs you probably should just use sub components. I agree with your concern re: complexity.

[nathansobo]

Yeah, that's the approach I've taken previously. My main concern is that if this breaks I'll have to take time away from improving Atom to deal with it. I'd be curious to hear more use cases.

[maxbrunsfeld]

Yeah, I don't have a strong opinion, but I'm inclined to agree with @larkin and the concerns about whether the complexity is worth it.

[lierdakil]

Well, one use-case when this could be handy (although only one-dimensionally) is when you have a list-view and want to scrollIntoView a particular item based on some sort of index. Currently it can be rather awkward (well, at least a particular way I figure this could be done -- via a data-index property and querySelector('item[data-index=${index}]') or similar). Maybe I'm missing something though?

[nathansobo]

@lierdakil You could create a subcomponent for each item and register it in a map that you pass in as a prop manually. That's what I've done in the past though it is a bit more boilerplate than this would be.

[nathansobo]

After discussing this, @BinaryMuse pointed out that React allows a function to be provided as the value of the ref property. That seems like an elegant solution to me. The function could be called with the target of the ref and you could store it in whatever way you see fit. I'd feel better about that.

[lierdakil]

IIRC, automatic refs property is also deprecated by React, i.e. setting ref to anything but function is discouraged nowadays. But you're right, this is an all-around better solution.