[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

kevinpschaaf · 2018-06-21T00:50:40Z

Add legacy-data-mixin as 1.x->2.x migration aide.

Mixin to selectively add back Polymer 1.x's undefined rules governing when observers & computing functions run based on all arguments being defined (reference https://www.polymer-project.org/1.0/docs/devguide/observers#multi-property-observers).

When polymer/lib/legacy/legacy-data-mixin.html is loaded at the app level, all legacy elements (defined with Polymer({...})) will have the mixin applied. The mixin only restores legacy data handling if _legacyUndefinedCheck: true is set on the element's prototype.

This mixin is intended for use to help migration from Polymer 1.x to 2.x+ by allowing legacy code to work during the migration period, while identifying observers and computing functions that need undefined checks to work without the mixin in Polymer 2. It is generally not intended for use in production.

Reference Issue

Fixes #5262.
Supersedes and closes #5263

kevinpschaaf · 2018-06-21T01:02:48Z

This is an alternate implementation of #5263, based on same concept there, with the goal of making the code to implement the legacy behavior purely opt-in. cc: @beckysiegel

kevinpschaaf · 2018-06-21T01:03:30Z

Two affordances were made in core to enable writing such a mixin:

Allow marshalArgs to be overridable via mixin my moving it from a private function to a protected function
Extend the API for Polymer.Class to allow a mixin to be applied prior to desugaring the info object into subclasses of the base LegacyElementMixin.

These seem like generally good changes on their own.

TimvdLippe

Some small nits. Other than that LGTM!

TimvdLippe · 2018-06-21T09:20:12Z

lib/legacy/legacy-data-mixin.html

+        if (this._legacyUndefinedCheck && args.length > 1) {
+          for (let i=0; i<args.length; i++) {
+            if (vals[i] === undefined) {
+              throw new UndefinedArgumentError(`argument '${args[i].name}' is undefined; ensure it has an undefined check`);


If I understand correctly, we are throwing an error here to break out of the control flow, as code later executed would erroneously run? If that is correct, maybe add a comment to make this explicit?

TimvdLippe · 2018-06-21T09:21:48Z

lib/legacy/legacy-data-mixin.html

+  });
+
+  const Class = Polymer.Class;
+  Polymer.Class = info => Class(info, Polymer.LegacyDataMixin);


This would break if a mixin was passed in to this function. Therefore, I think the mixin should be optional and the mixin should be conditionally applied to this mixin.

Ah good catch. The mixin option was a late addition.

Thanks, good catch. Updated.

TimvdLippe · 2018-06-21T09:24:49Z

test/unit/legacy-data.html

+        assert.equal(el.static.callCount, 1);
+        assert.equal(el.one.callCount, 1);
+        assert.equal(el.two.callCount, 0);
+        assert.isTrue(console.warn.calledOnce);


To make this more uniform, make this the same as line 239. E.g. el.two.callCount, 1

Yup, thanks.

TimvdLippe · 2018-06-21T09:28:07Z

Also the tests appear to fail on IE11, likely related to how it handles exceptions. And the latest version of shadydom appears to be now failing some of our tests. We have to look into these regressions separately.

beckysiegel · 2018-06-21T15:56:30Z

Thanks!

samccone · 2018-06-21T15:59:13Z

lib/legacy/legacy-data-mixin.html

+(function() {
+  'use strict';
+
+  const UndefinedArgumentError = class extends Error {};


If we want this error to have a meanifulful name this pattern should be followed https://azimi.me/2015/09/23/high-custom-error-class-es6.html

samccone · 2018-06-21T16:03:00Z

lib/mixins/property-effects.html

+       * @return {Array<*>} Array of argument values
+       * @private
+       */
+      _marshalArgs(args, path, props) {


this function has a non-trivial cyclical complexity score, can you please add unit tests to ensure the functionality is correct. Also it might be a sign that the logic should be split up into smaller more meaningful functions to maintain a higher degree of readability.

samccone · 2018-06-21T16:03:43Z

lib/mixins/property-effects.html

+       *
+       * @param {!Array<!MethodArg>} args Array of argument metadata
+       * @param {string} path Property/path name that triggered the method effect
+       * @param {Object} props Bag of current property changes


what is a bag? perhaps restate as a "hash" or key value collection of ...

samccone · 2018-06-21T16:04:06Z

test/unit/legacy-data.html

+</head>
+<body>
+
+  <script>


can you drop this since it is empty?

samccone · 2018-06-21T16:04:24Z

test/unit/legacy-data.html

+        document.body.removeChild(el);
+      });
+    });
+    // ------------------------------------------------


is this comment needed?

Did some refactoring

thanks, much nicer!

samccone · 2018-06-21T16:09:54Z

lib/legacy/legacy-data-mixin.html

+   * 
+   * When loaded, all legacy elements (defined with `Polymer({...})`)
+   * will have the mixin applied. The mixin only restores legacy data handling
+   * if `_legacyUndefinedCheck: true` is set on the element's prototype.


any reason why we are _ prefixing this prototype value?

As a convention, we generally underscore any property not intended as part of the DOM element's public API, and consider them "protected". https://www.polymer-project.org/2.0/docs/devguide/properties#private-and-protected-properties

samccone · 2018-06-21T16:12:13Z

test/unit/legacy-data.html

+        el.a = undefined;
+        el.b = undefined;
+        assert.equal(el.static.callCount, 1);
+        assert.equal(el.one.callCount, 2);


instead of callCount spy's can we sideeffect in the calls by incrementing a counter and then the tests are totally isolated without having to spy or mock on anything.

For example

foo(a, b) { this.fooCall++; }

samccone · 2018-06-21T16:13:14Z

test/unit/legacy-data.html

+  </test-fixture>
+
+  <script>
+  suite('imperative', () => {


This suite of tests has a lot of duplication I think we could do with some test helpers to significantly clean up the test implementation to make the code easier to maintain and easier to reason about.

samccone · 2018-06-21T16:13:47Z

test/unit/shady-content.html

-    if (!window.customElements) {
-      window.customElements = {};
+    if (window.customElements) {
+      customElements.forcePolyfill = true;


this change seems unrelated to the intent of the CL?

Separated out to #5268

vdonich · 2018-10-29T20:29:54Z

Looks like Computed bindings (https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-computed) are not handled by this mixin.
Here's jsfiddle that demonstrates it: https://jsfiddle.net/gaj0umLs/13/

kevinpschaaf · 2018-10-30T16:03:33Z

@vdonich Thanks for the report. I confirmed that TemplateInstances created from dom-if/dom-repeat don't get the LegacyDataMixin applied, hence the undefined check isn't running. Will look into a fix for that.

…i 10.

kevinpschaaf · 2018-10-31T23:10:28Z

Addressed the dom-if/dom-repeat issue pointed out by @vdonich here: 55e9dfd

vdonich · 2018-11-02T23:03:12Z

I've hit another snag when using wildcards.

Observer is being called despite undefined property value.
Change record is valid, but all value-related properties such as changeRecord.base and changeRecord.value are undefined:

{base: undefined, path: "alwaysUndefinedObject", value: undefined}

Here's the jsfiddle with example:
https://jsfiddle.net/gaj0umLs/26/

Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262.

c46a38d

kevinpschaaf requested review from dfreedm, sorvell and TimvdLippe as code owners June 21, 2018 00:50

googlebot added the cla: no label Jun 21, 2018

kevinpschaaf changed the base branch from master to 2.x June 21, 2018 00:50

Polymer deleted a comment from googlebot Jun 21, 2018

kevinpschaaf added cla: yes and removed cla: no labels Jun 21, 2018

kevinpschaaf added 3 commits June 20, 2018 17:58

Add legacy-data-mixin tests to runner.

4ec63a4

Ensure class.html is imported before patching it.

2214b9e

Update types.

7fc9564

kevinpschaaf added 2 commits June 20, 2018 18:35

Fix method to force CE polyfill on

b3bbd21

Fix test name

a470e45

TimvdLippe approved these changes Jun 21, 2018

View reviewed changes

samccone reviewed Jun 21, 2018

View reviewed changes

kevinpschaaf and others added 7 commits June 21, 2018 18:22

Add constructor name and fix mixin wrapping.

6818554

Factor out some helpers, add declarative tests.

08e13f2

Add comment

c550e9e

Merge branch '2.x' into 5262-kschaaf-legacy-undefined

61d1143

Merge branch '2.x' into 5262-kschaaf-legacy-undefined

b856b89

Fix update-types.

5992774

Ensure instanceof works after babel ES5 compilation

d44969a

kevinpschaaf changed the title ~~Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262.~~ [2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. Aug 21, 2018

kevinpschaaf added 9 commits October 31, 2018 10:16

Sync with changes made on master.

ed6deea

Merge branch '2.x' into 5262-kschaaf-legacy-undefined

763e40d

Update types.

7f2fcb1

Apply mixin to TemplatizeInstance

55e9dfd

Update wct for FF63, add FF62 to sauce for pre-WC testing, drop Safar…

509b73f

…i 10.

Update types.

75cfcb8

Use windows for FF62

9f65049

Add comment and handle undefined host case.

6e4f62d

Disable running FF in headless mode on Travis

d91ac93

sorvell approved these changes Oct 31, 2018

View reviewed changes

kevinpschaaf mentioned this pull request Nov 2, 2018

legacy-data-mixin: not applied to TemplatizerInstances #5422

Closed

kevinpschaaf merged commit 79de83c into 2.x Nov 2, 2018

kevinpschaaf deleted the 5262-kschaaf-legacy-undefined branch November 2, 2018 06:20

vdonich mentioned this pull request Nov 6, 2018

legacy-data-mixin doesn't work with wildcards observers #5428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

kevinpschaaf commented Jun 21, 2018 •

edited by TimvdLippe

kevinpschaaf commented Jun 21, 2018

kevinpschaaf commented Jun 21, 2018

TimvdLippe left a comment

TimvdLippe Jun 21, 2018

kevinpschaaf Jun 22, 2018

TimvdLippe Jun 21, 2018

kevinpschaaf Jun 21, 2018

kevinpschaaf Jun 22, 2018

TimvdLippe Jun 21, 2018

kevinpschaaf Jun 21, 2018

TimvdLippe commented Jun 21, 2018

beckysiegel commented Jun 21, 2018

samccone Jun 21, 2018

kevinpschaaf Jun 22, 2018

samccone Jun 21, 2018

samccone Jun 21, 2018

samccone Jun 21, 2018

samccone Jun 21, 2018

kevinpschaaf Jun 22, 2018

samccone Jun 22, 2018

samccone Jun 21, 2018

kevinpschaaf Jun 22, 2018

samccone Jun 21, 2018

samccone Jun 21, 2018

samccone Jun 21, 2018

kevinpschaaf Jun 22, 2018

samccone Jun 22, 2018

vdonich commented Oct 29, 2018

kevinpschaaf commented Oct 30, 2018

kevinpschaaf commented Oct 31, 2018

vdonich commented Nov 2, 2018 •

edited

[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

[2.x] Add legacy-data-mixin as 1.x->2.x migration aide. Fixes #5262. #5266

Conversation

kevinpschaaf commented Jun 21, 2018 • edited by TimvdLippe

Reference Issue

kevinpschaaf commented Jun 21, 2018

kevinpschaaf commented Jun 21, 2018

TimvdLippe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimvdLippe commented Jun 21, 2018

beckysiegel commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdonich commented Oct 29, 2018

kevinpschaaf commented Oct 30, 2018

kevinpschaaf commented Oct 31, 2018

vdonich commented Nov 2, 2018 • edited

kevinpschaaf commented Jun 21, 2018 •

edited by TimvdLippe

vdonich commented Nov 2, 2018 •

edited