Correctly handle commas in CP property keys #13231

nickiaconis · 2016-04-02T02:20:37Z

This addresses the issue of splitting on all commas in dependency key names, which prompted the opening of #13217.

Currently:

expandProperties('a,b,c.d.e,f', console.log); //=> 'a', 'b', 'c.d.e', 'f'

With this PR applied:

expandProperties('a,b,c.d.e,f', console.log); //=> 'a,b,c.d.e,f'

Edited to remove reference of nested brace expansion, since we don't intend to support it.

stefanpenner · 2016-04-02T02:22:29Z

packages/ember-metal/lib/expand_properties.js

-      current[index] = part;
-      all.push(current);
-    });
+  parts.forEach((part) => {


Map? Or should this be unrolled

Seems like it should be a straight for loop here.

Good point about the map, @stefanpenner.

Since there's an overall desire to reduce function calls, I'll rewrite this as a straight for loop.

rwjblue · 2016-04-02T02:33:02Z

I was under the impression that we did not intend to support nested brace expansion, so I'm not sure if we should add that.

@stefanpenner/@krisselden - Thoughts?

I'm also somewhat curious to see if we have made this better or worse performance wise, would it be possible to make a small benchmark before and after?

nickiaconis · 2016-04-02T03:19:47Z

@rwjblue I did not remember if we talked about the nesting in person or, if we did, what the conclusion was, and I didn't get a response about it over on #13217. It's possible to remove the second commit, which is the one that introduces nested handling, without affecting comma handling.

I'll put together a benchmark to compare before/comma/nested.

rwjblue · 2016-04-02T03:27:12Z

Awesome, that would be great, thank you.

rwjblue · 2016-04-10T17:17:51Z

@nickiaconis - Any updates here?

nickiaconis · 2016-04-11T23:31:37Z

Here are the results of the benchmarking. The code used to run the benchmark is also included in the gist.

Since the "comma" and "nested" versions were so much slower than the "before" version, I wrote a few other implementations. The "iteration" version is a little slower than the "before" version, but I think it's close enough to be acceptable.

rwjblue · 2016-04-12T00:18:33Z

Thanks for putting in the time on that benchmark! The iteration version looks good to me (implementation and perf wise).

@krisselden - What do you think?

nickiaconis · 2016-04-27T18:32:49Z

On April 12, @krisselden mentioned he was reviewing my benchmarks by trying them in a browser:

JIT stuff can make what you are testing not actually what you think
I just need to confirm them
I’m just not sure why they are so different

I wonder at what conclusion he arrived.

homu · 2016-06-15T19:59:24Z

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

stefanpenner · 2016-06-15T20:33:35Z

I am nervous about supporting this feature at all, Is it really common for people to valid properties with , in them?

nickiaconis · 2016-12-30T04:25:12Z

@stefanpenner I don't know if it's common, but it can definitely happen, especially when the property names are generated dynamically. For example, the place where I bumped into this was Erik's app-cache-demo from EmberConf 2016. The (static) data is set in the application route, and it has a model whose ID contains a comma. Later, that ID is used to create a computed property. Of course, this doesn't work because of the bug this PR aims to fix, hence the need to remove all commas from IDs in the component.

stefanpenner · 2016-12-30T20:06:17Z

The perf improvement in the iteration fix variant is very nice. I appreciate the due diligence here.

We (the ember core team) needs to decide if this is something we intend to support or not. As demonstrated it doesn't appear to make the status quo worse.

The only technical concern I can think of is, can we make the status quo better, and would enabling this feature prevent that?

@tomdale would love your guidance here.

tomdale · 2017-01-03T16:28:57Z

packages/ember-metal/lib/expand_properties.js

-    let part = parts[i];
-    if (part.indexOf(',') >= 0) {
-      properties = duplicateAndReplace(properties, part.split(','), i);
+  // Iterating backward over the pattern makes dealing with indeces easier.


Nitpick: s/indeces/indices/

tomdale · 2017-01-03T16:37:37Z

This seems like a great improvement to me. While it's easy to consider the previous ,-splitting behavior an edge case, it's infuriating to be able to express something valid in the language that isn't supported by your tools—and becomes the kind of thing that makes people think they don't like "magic."

My only comment is that I wouldn't mind having a few more tests to verify correct behavior in other, weird cases like nested {s, missing }s, etc. And I wonder if there is any performance benefit to having a fast path that checks for indexOf('}') and bails early if < 0? (There may not be, given the nature of the implementation.)

(And let me just say, @nickiaconis, I am very impressed that you implemented a stateful mini-parser here instead of trying to gin up an overly complicated regular expression, which seems like what most people try to do in these scenarios.)

nickiaconis · 2017-01-04T19:23:40Z

Thanks for the review, @tomdale!

Upstream changes have added tests for nested {s and missing }s. Now that I've rebased, those cases are covered. Were there any other tests you had in mind?

Speaking of upstream changes, an assertion to catch unbalanced and nested braces was added, which involves iterating over pattern, just as the mini-parser does. I've rolled that assertion into the mini-parser's iteration for a slight performance win. Do you agree with this choice? I've made it a separate commit for now, so it can be either squashed or pruned before merging.

The performance of pattern.indexOf is O(n), and the performance of the mini-parser when pattern.indexOf('{') === -1 is O(n), so there would be no performance benefit to bailing early. It might boost code clarity, but at the cost of a minor performance degradation when pattern.indexOf('{') !== -1, so I'd recommend against.

P.S. What's the rational behind the no-const-outside-module-scope ESLint rule?

rwjblue · 2017-01-04T19:26:47Z

P.S. What's the rational behind the no-const-outside-module-scope ESLint rule?

See "liberal let" in http://madhatted.com/2016/1/25/let-it-be.

nickiaconis · 2017-01-04T22:58:06Z

Thanks for the info @rwjblue! I'm of the "constantly const" opinion as articulated in https://ponyfoo.com/articles/var-let-const, but it's good to read a differing opinion.

homu · 2017-01-17T01:12:04Z

☔ The latest upstream changes (presumably #14830) made this pull request unmergeable. Please resolve the merge conflicts.

Roll unbalanced and nested brace assertion into mini-parser iteration

nickiaconis · 2017-02-02T19:54:19Z

@rwjblue I've rebased and resolved merge conflicts, so this is ready for review and/or merge.

rwjblue · 2017-02-02T20:46:55Z

LETS DO IT!

There was a bug in the brace expansion implementation before 2.13 that admitted expressions like `a.{b,c` and `a.b,c` in property expansion without failing. On February 2017, emberjs#13231 was merged improving the implementation of `expandProperties` and fixing the previous issue. Sadly, people upgrading from 2.12 are having problems with this. We have two options here: - Backporting emberjs#13231 fixes it and can be done, as of today, without any problem just by cherry-picking its two commits. Sadly, this would break people apps in a patch version, and this bug does not seem like a critical bugfix. - Adding a warning if a user is using invalid properties. I went with this option since it looks like the less intrusive one while letting the users know that their apps will break in the next update.

stefanpenner reviewed Apr 2, 2016
View reviewed changes

rwjblue mentioned this pull request Apr 9, 2016

CP dependency keys split on commas? #13217

Closed

nickiaconis force-pushed the cp-commas branch from 7e7763e to 28aa7be Compare April 12, 2016 00:27

nickiaconis force-pushed the cp-commas branch from 28aa7be to abe7a04 Compare June 2, 2016 19:58

kellyselden mentioned this pull request Jun 16, 2016

undefined behavior for nested brace expansion #13693

Closed

tomdale reviewed Jan 3, 2017

View reviewed changes

nickiaconis force-pushed the cp-commas branch from abe7a04 to f55afed Compare January 4, 2017 19:19

nickiaconis added 2 commits February 2, 2017 11:31

Preserve commas that are not part of a brace expansion

897ba43

fixup! Preserve commas that are not part of a brace expansion

14edb5c

Roll unbalanced and nested brace assertion into mini-parser iteration

nickiaconis force-pushed the cp-commas branch from f55afed to 14edb5c Compare February 2, 2017 19:50

rwjblue merged commit fc5be0b into emberjs:master Feb 2, 2017

jasonmit mentioned this pull request Mar 19, 2017

Change how dependent keys are registered on isDirty computed property toranb/ember-cli-simple-store#75

Merged

Serabe mentioned this pull request May 7, 2017

[BUGFIX LTS] Warn about invalid properties #15213

Merged

rwjblue mentioned this pull request Apr 27, 2018

Brace expansion should allow whitespace #16585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly handle commas in CP property keys #13231

Correctly handle commas in CP property keys #13231

nickiaconis commented Apr 2, 2016

stefanpenner Apr 2, 2016

rwjblue Apr 2, 2016

nickiaconis Apr 2, 2016

rwjblue commented Apr 2, 2016

nickiaconis commented Apr 2, 2016

rwjblue commented Apr 2, 2016

rwjblue commented Apr 10, 2016

nickiaconis commented Apr 11, 2016

rwjblue commented Apr 12, 2016

nickiaconis commented Apr 27, 2016

homu commented Jun 15, 2016

stefanpenner commented Jun 15, 2016

nickiaconis commented Dec 30, 2016 •

edited

Loading

stefanpenner commented Dec 30, 2016

tomdale Jan 3, 2017

tomdale commented Jan 3, 2017 •

edited

Loading

nickiaconis commented Jan 4, 2017

rwjblue commented Jan 4, 2017

nickiaconis commented Jan 4, 2017

homu commented Jan 17, 2017

nickiaconis commented Feb 2, 2017

rwjblue commented Feb 2, 2017

Correctly handle commas in CP property keys #13231

Correctly handle commas in CP property keys #13231

Conversation

nickiaconis commented Apr 2, 2016

stefanpenner Apr 2, 2016

Choose a reason for hiding this comment

rwjblue Apr 2, 2016

Choose a reason for hiding this comment

nickiaconis Apr 2, 2016

Choose a reason for hiding this comment

rwjblue commented Apr 2, 2016

nickiaconis commented Apr 2, 2016

rwjblue commented Apr 2, 2016

rwjblue commented Apr 10, 2016

nickiaconis commented Apr 11, 2016

rwjblue commented Apr 12, 2016

nickiaconis commented Apr 27, 2016

homu commented Jun 15, 2016

stefanpenner commented Jun 15, 2016

nickiaconis commented Dec 30, 2016 • edited Loading

stefanpenner commented Dec 30, 2016

tomdale Jan 3, 2017

Choose a reason for hiding this comment

tomdale commented Jan 3, 2017 • edited Loading

nickiaconis commented Jan 4, 2017

rwjblue commented Jan 4, 2017

nickiaconis commented Jan 4, 2017

homu commented Jan 17, 2017

nickiaconis commented Feb 2, 2017

rwjblue commented Feb 2, 2017

nickiaconis commented Dec 30, 2016 •

edited

Loading

tomdale commented Jan 3, 2017 •

edited

Loading