chore(dialog): change animation properties to methods

[vdegenne]

Moving getOpenAnimation and getCloseAnimation properties to methods type for the doc:

[asyncLiz]

Hm, conceptually I think they're still properties in the sense that they're expected to be written, unlike a method which is usually expected not to be changed.

Open to arguments why they should be considered methods though!

[vdegenne]

Sorry I don't really have any arguments but for the sake of learning I want to ask why are they considered functions then? why not openAnimation and closeAnimation directly?

[asyncLiz]

Sorry I don't really have any arguments but for the sake of learning I want to ask why are they considered functions then? why not openAnimation and closeAnimation directly?

There's little technical difference between them. Methods share the same prototype function implementation, while properties are unique per instance.

class Foo {
  propFunction = () => true;

  methodFunction() { return true; }
}

const fooOne = new Foo();
const fooTwo = new Foo();

console.log(fooOne.propFunction === fooTwo.propFunction); // false
console.log(fooOne.methodFunction === fooTwo.methodFunction); // true

Also the methodFunction can also be called on the prototype while the propFunction is harder to call without an instance... but if you're in Foo.prototype.methodFunction.call(...) territory you got bigger problems.

getOpenAnimation and getCloseAnimation are intended to be replaced by the user, while methods on classes are not typically intended to be replaced. So informing tools like lit-analyzer that it's a property can be beneficial.

Technically though, yes, changing it to a shared function instance should theoretically be more performant since we're not creating a new JS function instance per class instance. I doubt there's a measurable impact though.

[vdegenne]

Oops sorry @asyncLiz I meant why they should be functions and not an object directly?

class Foo {
  openAnimation = DIALOG_DEFAULT_OPEN_ANIMATION
  // instead of getOpenAnimation() => DIALOG_DEFAULT_OPEN_ANIMATION
}

[asyncLiz]

Oops sorry @asyncLiz I meant why they should be functions and not an object directly?

I originally made them functions so that users could have more options to create dynamic animations.

dialog.getOpenAnimation = () => {
  if (isM3()) {
    return DIALOG_DEFAULT_OPEN_ANIMATION;
  }

  // A different animation for an app that is
  // iteratively upgrading from M2 to M3
  return M2_MIGRATION_DIALOG_OPEN_ANIMATION;
};

This is just my opinion on what's useful. Would love to hear your dev experience on how you're typically overriding the animations! Lit binding, setting the prop directly, extending, etc.

We could make them both properties and methods if it's convenient devx to bind an object. For example, in a lit binding it's more verbose and creates a function:

html`
  <md-dialog .openAnimation=${OPEN_ANIMATION}>
  <!-- vs -->
  <md-dialog .getOpenAnimation=${() => OPEN_ANIMATION}>
`;

[vdegenne]

It just caught my eyes in the docs that's all, I think functions are more handy yes. But in that case we might want to add the keyword await when we call it internally?

[asyncLiz]

I'm not sure I know a good use case for why animation generation would need to be asynchronous to justify introducing await task timing complexities.