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.

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

Conversation

beckysiegel
Copy link
Contributor

@beckysiegel beckysiegel commented Jun 18, 2018

When an computed property or complex observer is triggered and the calling element has a property 'usePolymer1StyleObserver' set to true, require all arguments to be defined to call the function.

Devs can set their v1 hybrid elements to have this property for a smoother incremental upgrade to Polymer2 (Not meant to be included in Polymer3)

Reference Issue

#5262

When an computed property or complex observer is triggered and the calling element has a property 'usePolymer1StyleObserver' set to true , require all arguments to be defined to call the function. Elements can set their v1 hybrid elements to have this property in order to more incrementally upgrade to Polymer2. This is meant to be in Polymer2 only and not Polymer3, as a aid to incrementally update apps that will have many changes to their observers.

Update property-effects.html

Update property-effects.html

Update property-effects.html
@kevinpschaaf
Copy link
Member

Thanks! I'm going to investigate refactoring the necessary functions to be overridable via a mixin to add this behavior. If that looks possible, I'll make an updated PR.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants