Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update property-effects.html #5263

Closed
wants to merge 1 commit into from
Closed

Update property-effects.html #5263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/mixins/property-effects.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,14 @@
let fn = context[info.methodName];
if (fn) {
let args = marshalArgs(inst.__data, info.args, property, props);
if (inst.usePolymer1StyleObserver) {
for (const arg of args) {
if (arg === undefined) {
console.warn(`arg is not defined, ensure it has an undefined check`);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this prevents the method from running, it will result in undefined being returned from runMethodEffect, and both runComputedEffect and runBindingEffect use the return value to take further action (assign to a host property and bound element's property, respectively), which would be another change in behavior. So the flag technically needs to prevent those side-effects in a couple of other places for behavior parity with 1.x.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm 🤔, on second thought: in Polymer 2, users won't be able to precisely match the Polymer 1 behavior (not assigning to computing functions) themselves by manually adding undefined argument checks to their methods. So perhaps it is better to not try to match it when the flag is enabled, since any issues due to it would still be issues once the flag is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an example of a time where the behavior would be different?

Copy link
Member

@kevinpschaaf kevinpschaaf Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty obscure... It would be something like this:

<dom-module is="x-host">
  <template>
    <x-child prop="[[computeSomething(a, b)]]"></x-child>
  </template>
  <script>
  Polymer({
    is: 'x-host',
    ...
    computeSomething(a, b) {
      return a+b;
    }
  });
  </script>
</dom-module>

In Polymer 1:

xHost.a = 1;         // Does nothing
xHost.b = 2;         // Sets xChild.prop = 3;
xHost.a = undefined; // Does nothing

In Polymer 2, by returning early from runMethodEffect (and thus returning undefined as the computed value):

xHost.a = 1;         // Sets xChild.prop = undefined;
xHost.b = 2;         // Sets xChild.prop = 3;
xHost.a = undefined; // Sets xChild.prop = undefined;

... since a return value of undefined from runMethodEffect doesn't abort the effect of the binding; it just sets that value (undefined).

Whether the difference would be observable depends on how x-child observes prop, and what legacy undefined rules it hit in there. In the end, I'm not sure the difference matters; just getting the warning and knowing to do something sensible re: checking for undefined is going to solve most issues I think.

}
}
}
return fn.apply(context, args);
} else if (!info.dynamicFn) {
console.warn('method `' + info.methodName + '` not defined');
Expand Down