CP dependency keys split on commas?

[nickiaconis]

Is this a thing we (I?) can deprecate? It would be nice to remove the extra loop from that function, and it would also open up the ability to use key names with commas (this bit me recently). I don't think this functionality is documented in the guides/api, but I didn't look super closely.

[nickiaconis]

/cc @rwjblue @nathanhammond

[nickiaconis]

"That function" being https://github.com/emberjs/ember.js/blob/v2.4.0/packages/ember-metal/lib/expand_properties.js#L48-L52 of course. 😓

[rwjblue]

Using brace expansion was a feature added sometime in 1.5ish timeframe.

name: computed('model.{last,first}', function() {
  let { last, first } = this.getProperties('last', 'first');
  return first + ' ' + last;
})

It seems to me that that comma detection/handling is what is supporting this feature, am I missing something?

[nathanhammond]

I had no idea that was a feature.

1. We should at least do this without forEach for such a common code path.
2. If we can show significant perf wins removing this would that be grounds for deprecation of the sugar?
3. Alternatively we could probably do a transform at compile time to drop the runtime code and still have the feature.

[rwjblue]

I think the 1) is a great idea (there are two forEach's in that file that can be removed).

I think we are pretty fond of the feature in general, so I think 2) is kinda out, but expanding at build time seems possible.

[nickiaconis]

Interesting. I too was not aware of this feature. Do you know if/where it's documented? Might we want to only split on commas that are inside curly braces? That is 'model.{first,last}' gets split, but 'ember-cli,-the-next-generation' does not.

Also, I can put together a PR for 1). A question that comes to mind is, would 'person.{name.{first,last},twitterHandle}' be expected expand to expand to 'person.name.first', 'person.name.last', 'person.twitterHandle' or does this only go one level deep?

[acorncom]

If we do remove this, it'd be helpful to know over on the guides as well, as we just merged a PR to put it into the guides ;-) See emberjs/guides#1348 for more detail (and a link back to the original blog post where it was introduced)

[stefanpenner]

• This isn't going to be removed.
• Moving to for loops seem fine
• this is done at design time, and is quite unlikely to pose much of a performance hit especially relative to things like extend and merge mixins etc. If someone can demonstrate this to be incorrect, a Babel plugin could be created. That being said, I would believe the overhead (although it wouldn't surprise me if some small TLC is required) to be negligible especially relative to the above mentioned points.

[krisselden]

@stefanpenner that was the original idea, but if you define a lot of classes design time starts being an issue for cold boot.

Last time I was in this code it seemed like there was a lot of cleanup that could be done.

@nickiaconis did you actually notice a particular issue with this? In quite a few cases simple closures in forEach inline.

[stefanpenner]

To direct the energy someone that will absolutely yield a big win (and fix some bugs we have a hard time fixing without perf issues). Pre compiling mixins (all arguments to extend) to prevent the need for mixing merging needing to detect each member, would be rad.

[nickiaconis]

@acorncom Fantastic! Is anyone working on getting this into the API docs as well?

@stefanpenner It would appear this has already been migrated to for loops.

@krisselden There is a bug with the logic behind this. It splits on all commas, even those that are not part of a brace expansion. I'm working on a fix, but there are different ways to take it depending on whether nested brace expansions are supported or not. Should they be?

expandProperties('foo.{bar.{fiz,baz},[]}', console.log) //=> 'foo.bar.fiz', 'foo.bar.baz', 'foo.[]'

[stefanpenner]

@krisselden I would love to see cases where forEach in lines. Or do you just mean the captured context is dropped and don't itself cost

[pixelhandler]

@nickiaconis Nice initiative on working on a feature. I'm curious about using the RFC process. That may help with getting the feature vetted as well as provide background for documentation for the guides/api docs, see CONTRIBUTING.md#requesting-a-feature which recommends creating an RFC Issue to request a feature/enhancement (no need for a PR with RFC, only an issue in the repo)

[nathanhammond]

@pixelhandler This feature already exists and is not well documented. There are also bugs in the feature, but because it isn't documented nobody is sure how it should be implemented.

@stefanpenner @krisselden @rwjblue Can somebody make an executive decision on the full microsyntax for brace expansion that is 100% backwards compatible with the current broken implementation?

[rwjblue]

@nathanhammond - I do not understand your comment, @nickiaconis submitted #13231 to address this issue and I believe that we are all on the same page over there (though there is still some cleanup that he was working on).

[rwjblue]

Actually, I'm going to close this in favor of the #13231 (to keep further conversation together....

[nathanhammond]

@rwjblue I was clarifying that this was a bug fix, not a feature request. That is all.