[FEATURE property-brace-expansion-improvement] Potential improvement as per #4605

[gordonkristan]

I love the property brace expansion introduced in 1.4, but on several occasions I've entered properties with an incorrect syntax. So when @zzarcon mentioned expanding the functionality, I took a stab at it.

Property brace expansion will now work properly with any number of braces in any location, provided that the braces are surrounded by periods and only contain a comma separated list of valid property names (no whitespace).

You can look at the code for examples, but here's the gist:

'foo.{bar,baz}.@each' => 'foo.bar.@each', 'foo.baz.@each'
'{foo,bar}.{spam,eggs}' => 'foo.spam', 'foo.eggs', 'bar.spam', 'bar.eggs'
'{foo}.bar.{baz}' => 'foo.bar.baz'

As far as the algorithm to do the expansion goes, I'm 100% positive that there's a more efficient way. But this way is fairly short and relatively easy to understand, which I think trumps efficiency in this case. However, I'd be glad to improve the algorithm (or anything else) if requested.

[zzarcon]

Amazing man! this is exactly what I want, I think that with the new property brace expansion the dependent keys will be more declaratives and less verbose.

👍 for you!

[stefanpenner]

@hjdivad you did the initial implementation of our current expansion can you +1/-1 ?

[stefanpenner]

also this should be labeled [FEATURE <something related to the feature>] with the appropriate feature.json and feature.md additions.

[gordonkristan]

How exactly would I put this behind a feature flag? It improves (replaces) existing stable functionality, so I can't disable it without disabling the old functionality too. Should I include both implementations and have it choose the correct one at runtime?

[hjdivad]

Would it not be simpler to splice new entries rather than duplicateAndReplace?

[gordonkristan]

I did the implementation rather quickly, just to test the waters. I'll go ahead and improve the implementation a bit and put it behind a feature flag.

[hjdivad]

@gordonkristan yeah exactly; if feature then newImpl else oldImpl.

I'm 👍 on the feature. I'm slightly confused about the implementation, but maybe there's some good reason why it can't just use splice?

[gordonkristan]

I updated the code to put it behind feature flags. However, I'm a little confused on how I could use splice to achieve what I want. Right now, here's my logic:

The parts array that comes from splitting the original string could look something like this:

['a.', 'b,c', '.d.', 'e,f']

So when I'm iterating the parts, I get to the second element of the array. From that point, I need to duplicate the array because the property can't be properly formed until I expand the last element of the array as well. So I have no choice but to turn the above array into the following, and then continue iterating until I get to the last element which also needs expanding.

['a.', 'b', '.d.', 'e,f']
['a.', 'c', '.d.', 'e,f']

I could use splice on the original array, but I would still need to duplicate it at least once to get the second array.

So that's my reasoning for using the duplicateAndReplace method. (BTW, that process could easily be done using strings instead of arrays, but I found using arrays easier.) If I'm missing something obvious, just let me know. :)

[GetContented]

@gordonkristan

I think this is the intended algorithm, partially thought out, to give you the gist:

- say initialString = 'foo.{bar,baz,jo}.harry.{bit,bop}'
- set segments =  initialString.split('.') then delete '{' and '}' => ["foo", "bar,baz,jo", "harry", "bit,bop"]
- expandedItemsCollectorArray = []
- loop over segments as segment
  -- segmentItems = segment.split(',')
  -- duplicate the items in expandedItemsCollectorArray so there are segmentItems.count many copies of the entire set (or if there are none, just add them), otherwise:
  -- loop over segmentItems as segmentItem with index
    --- splice each segmentItem to the end of each expandedItemsCollectoarArray with a dot in front

And if you used a regexp or two, you could avoid all the splits.

[gordonkristan]

@JulianLeviston I think that's pretty much what I'm doing, only I'm just replacing the item in the array rather than using splice. I'm looping over segments just as you do and I'm looping over segmentItems just as you do. So it's definitely got the same run complexity.

[GetContented]

Apologies! :) It seemed simpler to my brain for some reason hehe... nevermind then... :)

[hjdivad]

@gordonkristan ah good point about needing to do duplication either way. You are right, the simplification I was thinking about with splice is not correct. Your implementation seems fine to me.

[rwjblue]

@gordonkristan - Could you rebase?

@hjdivad - Am I understanding your last comment correctly as a +1?

[gordonkristan]

I rebased, so I think it's ready to merge now.

[pangratz]

I think you have to explicitly write if (!Ember.FEATURES.isEnabled('property-brace-expansion-improvement')) { here, as otherwise this won't work with the defeatureify tool ...

cc @rjackson

[tomdale]

This was go'ed for 🚀 at the core team meeting. We would like to add some documentation to the guides that warns people about how easy it is to add a combinatorial explosion of dependencies.