[3.x] Add legacy-data-mixin as 1.x->2.x/3.x migration aide. Fixes #5262. #5339
Conversation
kevinpschaaf
requested review from
dfreedm,
sorvell and
TimvdLippe
as code owners
kevinpschaaf
changed the title
lib/legacy/legacy-data-mixin.js
Outdated
| constructor(message) { | ||
| super(message); | ||
| this.name = this.constructor.name; | ||
| // Affordances for ensuring instanceof works after babel ES5 compilation |
There was a problem hiding this comment.
Surprised this is needed. Verify?
There was a problem hiding this comment.
The version of babel we use in CLI doesn't handle subclassing builtins correctly; the latest one does, so will add TODO to remove once CLI updates to latest babel.
There was a problem hiding this comment.
Note: should be fixed by Polymer/tools#670
lib/legacy/legacy-data-mixin.js
Outdated
| * @private */ | ||
| class LegacyDataMixin extends superClass { | ||
| /** | ||
| * Overrides `Polyer.PropertyEffects` to add `undefined` argument |
lib/legacy/legacy-data-mixin.js
Outdated
| */ | ||
| _marshalArgs(args, path, props) { | ||
| const vals = super._marshalArgs(args, path, props); | ||
| if (this._legacyUndefinedCheck && args.length > 1) { |
There was a problem hiding this comment.
Add comment briefly explaining the rules so it's clear that single argument observers are always called.
lib/legacy/legacy-data-mixin.js
Outdated
| if (vals[i] === undefined) { | ||
| // Break out of effect's control flow; will be caught in | ||
| // wrapped property effect function below | ||
| throw new UndefinedArgumentError(`argument '${args[i].name}' is undefined; ensure it has an undefined check`); |
There was a problem hiding this comment.
Suggest making the error use sentence casing and periods.
lib/legacy/legacy-data-mixin.js
Outdated
| _marshalArgs(args, path, props) { | ||
| const vals = super._marshalArgs(args, path, props); | ||
| if (this._legacyUndefinedCheck && args.length > 1) { | ||
| for (let i=0; i<args.length; i++) { |
There was a problem hiding this comment.
Seems like it would be more clear to loop over vals?
lib/legacy/legacy-data-mixin.js
Outdated
| const fn = effect.fn; | ||
| effect.fn = function() { | ||
| try { | ||
| fn.apply(this, arguments); |
There was a problem hiding this comment.
Can we just check the arguments here and avoid the need to try catch the error? Then you don't even need to expose _marshalArgs.
There was a problem hiding this comment.
No, needs to also abort computed properties & inline computing functions. Will add tests for those.
test/unit/legacy-data.html
Outdated
| }, | ||
| _legacyUndefinedCheck: true, | ||
| observers: [ | ||
| 'staticObserver("staticObserver")', |
There was a problem hiding this comment.
Shouldn't we also test inline template expressions that have multiple arguments?
lib/legacy/legacy-data-mixin.js
Outdated
|
|
||
| }); | ||
|
|
||
| Polymer.Class = (info, mixin) => Class(info, |
There was a problem hiding this comment.
Why not just put LegacyDataMixin on top of the user class?
There was a problem hiding this comment.
Ok, the answer is no because the mixin customizes how properties are created on the element prototype. Since a subclass only creates properties that it defines, it will not modify properties created by its superclass.
| * @return {Array<*>} Array of argument values | ||
| * @private | ||
| */ | ||
| _marshalArgs(args, path, props) { |
There was a problem hiding this comment.
Did you consider altering this method to get the info needed to present a good error? I'm concerned that since the intension is to warn, but the code throws here, we're depending on catching that where it's called.
There was a problem hiding this comment.
Ok, disregard that. It's clear that we need to abort the effect of marshaling the arguments and throwing let's us do this.
Add legacy-data-mixin as 1.x->2.x/3.x migration aide.
Mixin to selectively add back Polymer 1.x's
undefinedrules 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.jsis loaded at the app level, all legacy elements (defined withPolymer({...})) will have the mixin applied. The mixin only restores legacy data handling if_legacyUndefinedCheck: trueis 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/3. It is generally not intended for use in production.
Reference Issue
Fixes #5262.
Supersedes and closes #5263