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

Consider renaming renderCallback function (to render) #5

Closed
filipbech opened this issue Oct 26, 2017 · 6 comments
Closed

Consider renaming renderCallback function (to render) #5

filipbech opened this issue Oct 26, 2017 · 6 comments

Comments

@filipbech
Copy link
Contributor

I think you should consider renaming the render-function to be just render, to align with libraries like react (and Justins example from CDS).
its not a big deal, but easier now than when this catches on..

(great work btw)

@kenchris
Copy link
Owner

I choose the name because it was consistent with other web components callbacks like connectedCallback(). Maybe @justinfagnani has opinions?

@filipbech
Copy link
Contributor Author

in his example at cds he used render

https://twitter.com/justinfagnani/status/922966799922094080

@kenchris
Copy link
Owner

I saw that, but that doesn't mean a lot of though was giving about that name :-)

Like it could matter to be consistent with web components, or it might even be bad as it makes standardization harder (have to avoid clashes). I am fine both ways, but I think it is good to have the discussion early

@filipbech
Copy link
Contributor Author

I agree... and great work btw... litElement is super useful

@justinfagnani
Copy link

I did put some thought into using render() as opposed to renderCallback().

In one set of mixins I was using, invalidate simply called an abstract renderCallback() method, which didn't return anything:

const Render = (base) => class extends base {
  async invalidate() {
    if (!this.needsRender) {
      this.needsRender = true;
      this.needsRender = await false;
      this.renderCallback();
    }
  }
}

And then the LitRender mixin implemented renderCallback():

const RenderLit = (base) => class extends base {
  renderCallback() {
    render(this.render(), this.shadowRoot);
  }
}

The reason for this split is 1) the platform's *Callback methods don't return anything. 2) Any template mixin can implement renderCallback() and because it doesn't have a return value, it has the simplest contract possible, and isn't tied to a particular template library. 3) render() for the lit-html mixin specifically is simpler and more similar to React, so possibly easier to understand by React users.

@filipbech
Copy link
Contributor Author

PR here: #7

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

No branches or pull requests

3 participants