refactor(data): camelCase keys in jqLite#data #15238

mgol · 2016-10-10T00:01:13Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
A behavior change.

What is the current behavior? (You can also link to an open issue here)
jqLite#data uses keys unchanged.

What is the new behavior (if this is a feature change)?
jqLite#data camelCases keys in its getters/setters. This aligns jqLite with jQuery.

Does this PR introduce a breaking change?
Yes.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

This is WIP; this might slightly decrease performance so I wanted to discuss it first.

Also, finishing this would require making our camelCase function more strict so I'd like to discuss it first but I think we should do that anyway; the -moz- hack doesn't really belong here.

petebacondarwin

If performance is a concern then we ought to run some benchmarking...
Can we make camelCase() more efficient in the common case?

petebacondarwin · 2016-10-10T05:20:50Z

test/jqLiteSpec.js

+
+    fdescribe('camelCasing keys', function() {
+      // jQuery 2.x has different behavior; skip the tests.
+      // if (isJQuery2x()) return;


commented out?

petebacondarwin · 2016-10-10T05:24:21Z

test/jqLiteSpec.js

+        var element = jqLite(a);
+
+        element.data('a-B-c-d-42--e', 'z-x');
+        expect(element.data()).toEqual({'a-BCD-42-E': 'z-x'});


is this actually how we want the camelcasing behaviour to work when there are already capital letters and double hyphens in the key? Or is it because of our current implementation of camelCase()?

This is exactly as jQuery does it. Only a single dash + a lowercase letter get converted.

This is also how the HTML5 spec converts data attribute names to keys in dataset. That's where jQuery took it from.

It's also nice in that it's fully 1-1, the conversion doesn't lose any information.

mgol · 2016-10-12T12:53:08Z

The first (new) commit is the commit from PR #15249, it's a prerequisite to make the second commit pass the tests.

mgol · 2016-10-12T14:38:28Z

I updated the commit message, adding a breaking change info. I'm not very happy about the migration steps there, any better ideas?

gkalpak · 2016-10-12T15:17:13Z

src/jqLite.js

        } else { // mass-setter: data({key1: val1, key2: val2})
-          extend(data, key);
+          for (prop in key) {


This is different than extend:

extend only copies own properties.

extend ignores $$hashKey.

If the chenges are intentional (e.g. to align with jQuery), we should clearly document them in the breaking change notice.

Ok. Relevant jQuery code: https://github.com/jquery/jquery/blob/3.1.1/src/data/Data.js#L63-L65

gkalpak · 2016-10-12T15:24:51Z

The migration steps for that usecase work for me. I would also add an example of how this affects interacting with the data retrieved with a mass-getter. E.g.:

// Before:
elem.data('foo-bar', 42);
elem.data()['foo-bar'];

// After:
elem.data('foo-bar', 42);
elem.data()['fooBar'];

petebacondarwin · 2016-10-12T15:24:54Z

Both these commits should be scoped as fix. We are "fixing" the behaviour to match that of JQuery; but also with this the changes would not appear in the CHANGELOG automatically.

petebacondarwin · 2016-10-12T15:26:45Z

I am not clear about what the migration step in 99b2dde is saying. Why are there more items in the "before" version? It would be helpful if the migration described what the "before" was trying to achieve and then how the "after" achieves it.

mgol · 2016-10-12T17:50:24Z

I am not clear about what the migration step in 99b2dde is saying. Why are there more items in the "before" version?

I meant that either one of them may be replaced by either one of the cases in the "After" section. Can we have multiple before/after pairs?

petebacondarwin · 2016-10-12T19:27:55Z

OK, so it would be good if you said this explicitly in the commit message.

jqLite needs camelCase for it's css method; it should only convert one dash followed by a lowercase letter to an uppercase one; it shouldn't touch underscores, colons or collapse multiple dashes into one. This is behavior of jQuery 3 as well. This commit separates jqLite camelCasing from the $compile one (and $sce but that's an internal-only use). The $compile version should behave as before. Also, jqLite's css camelCasing logic was put in a separate function and refactored: now the properties starting from an uppercase letter are used by default (i.e. Webkit, not webkit) and the only exception is for the -ms- prefix that is converted to ms, not Ms. This makes the logic clearer as we're just always changing a dash followed by a lowercase letter by an uppercase one; this is also how it works in jQuery. Ref angular#15126 Fix angular#7744 BREAKING CHANGE: before, when Angular was used without jQuery, the key passed to the css method was more heavily camelCased; now only a single (!) hyphen followed by a lowercase letter is getting transformed. This also affects APIs that rely on the css method, like ngStyle. If you use Angular with jQuery, it already behaved in this way so no changes are needed on your part. To migrate the code follow the example below: Before: HTML: // All five versions used to be equivalent. <div ng-style={background_color: 'blue'}></div> <div ng-style={'background:color': 'blue'}></div> <div ng-style={'background-color': 'blue'}></div> <div ng-style={'background--color': 'blue'}></div> <div ng-style={backgroundColor: 'blue'}></div> JS: // All five versions used to be equivalent. elem.css('background_color', 'blue'); elem.css('background:color', 'blue'); elem.css('background-color', 'blue'); elem.css('background--color', 'blue'); elem.css('backgroundColor', 'blue'); // All five versions used to be equivalent. var bgColor = elem.css('background_color'); var bgColor = elem.css('background:color'); var bgColor = elem.css('background-color'); var bgColor = elem.css('background--color'); var bgColor = elem.css('backgroundColor'); After: HTML: // Previous five versions are no longer equivalent but these two still are. <div ng-style={'background-color': 'blue'}></div> <div ng-style={backgroundColor: 'blue'}></div> JS: // Previous five versions are no longer equivalent but these two still are. elem.css('background-color', 'blue'); elem.css('backgroundColor', 'blue'); // Previous five versions are no longer equivalent but these two still are. var bgColor = elem.css('background-color'); var bgColor = elem.css('backgroundColor');

This change aligns jqLite with jQuery 3. Close angular#15126 BREAKING CHANGE: Previously, keys passed to the data method were left untouched. Now they are internally camelCased similarly to how jQuery handles it, i.e. only single (!) hyphens followed by a lowercase letter get converted to an uppercase letter. This means keys `a-b` and `aB` represent the same data piece; writing to one of them will also be reflected if you ask for the other one. If you use Angular with jQuery, it already behaved in this way so no changes are required on your part. To migrate the code follow the examples below: BEFORE: /* 1 */ elem.data('my-key', 2); elem.data('myKey', 3); /* 2 */ elem.data('foo-bar', 42); elem.data()['foo-bar']; // 42 elem.data()['fooBar']; // undefined /* 3 */ elem.data()['foo-bar'] = 1; elem.data()['fooBar'] = 2; elem.data()['foo-bar']; // 1 AFTER: /* 1 */ // Rename one of the keys as they would now map to the same data slot. elem.data('my-key', 2); elem.data('my-key2', 3); /* 2 */ elem.data('foo-bar', 42); elem.data()['foo-bar']; // undefined elem.data()['fooBar']; // 42 /* 3 */ elem.data()['foo-bar'] = 1; elem.data()['fooBar'] = 2; elem.data()['foo-bar']; // 2

mgol · 2016-10-12T22:48:53Z

Both these commits should be scoped as fix

I fixed the first commit in #15249 and the second here & rebased. PTAL.

mgol · 2016-10-12T22:49:15Z

@gkalpak PTAL as well, I added sth like what you suggested.

petebacondarwin · 2016-10-13T06:47:45Z

Landed as fc0c11d and 73050cd

mgol added component: jqLite type: refactor needs: breaking change labels Oct 10, 2016

mgol added this to the 1.6.0-rc.0 milestone Oct 10, 2016

mgol self-assigned this Oct 10, 2016

googlebot added the cla: yes label Oct 10, 2016

petebacondarwin reviewed Oct 10, 2016

View reviewed changes

mgol force-pushed the data branch 2 times, most recently from a1fa313 to ab465de Compare October 12, 2016 12:52

mgol changed the title ~~[WIP] refactor(data): camelCase keys in jqLite#data~~ refactor(data): camelCase keys in jqLite#data Oct 12, 2016

mgol force-pushed the data branch 4 times, most recently from 20bd769 to 72b195b Compare October 12, 2016 14:36

gkalpak suggested changes Oct 12, 2016

View reviewed changes

mgol added 2 commits October 13, 2016 00:45

mgol force-pushed the data branch from 72b195b to 9921e7f Compare October 12, 2016 22:48

petebacondarwin closed this Oct 13, 2016

petebacondarwin mentioned this pull request Oct 13, 2016

refactor(*): separate jqLite/compile/sce camelCasing logic #15249

Closed

3 tasks

mgol deleted the data branch October 13, 2016 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(data): camelCase keys in jqLite#data #15238

refactor(data): camelCase keys in jqLite#data #15238

mgol commented Oct 10, 2016

petebacondarwin left a comment

petebacondarwin Oct 10, 2016

petebacondarwin Oct 10, 2016

mgol Oct 12, 2016

mgol Oct 12, 2016

petebacondarwin Oct 13, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

gkalpak Oct 12, 2016

mgol Oct 12, 2016

gkalpak commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 13, 2016

refactor(data): camelCase keys in jqLite#data #15238

refactor(data): camelCase keys in jqLite#data #15238

Conversation

mgol commented Oct 10, 2016

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Oct 10, 2016

Choose a reason for hiding this comment

petebacondarwin Oct 10, 2016

Choose a reason for hiding this comment

mgol Oct 12, 2016

Choose a reason for hiding this comment

mgol Oct 12, 2016

Choose a reason for hiding this comment

petebacondarwin Oct 13, 2016

Choose a reason for hiding this comment

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

gkalpak Oct 12, 2016

Choose a reason for hiding this comment

mgol Oct 12, 2016

Choose a reason for hiding this comment

gkalpak commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 12, 2016

mgol commented Oct 12, 2016

mgol commented Oct 12, 2016

petebacondarwin commented Oct 13, 2016