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

Render any value returned by #render. #712

Merged
merged 12 commits into from
Jan 16, 2020
Merged

Render any value returned by #render. #712

merged 12 commits into from
Jan 16, 2020

Conversation

bicknellr
Copy link
Member

@bicknellr bicknellr commented Jun 13, 2019

As of lit/lit#910, render accepts any type. This PR causes LitElement to render anything returned by render.


Fixes #618, Fixes #168

@43081j
Copy link
Contributor

43081j commented Jul 15, 2019

can you also change the base render method to have a return type of unknown rather than TemplateResult|void?

@justinfagnani
Copy link
Contributor

@bicknellr is this ready to go in now?

@bicknellr bicknellr marked this pull request as ready for review September 5, 2019 23:18
* DOM.
* @param {TemplateResult} Template to render.
* Render method used to render the value to the element's DOM.
* @param {unknown} The value to render.
Copy link
Contributor

Choose a reason for hiding this comment

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

These need the property names as the first word following the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

*/
protected render(): TemplateResult|void {
protected render(): unknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elide the return if you type this as unknown|void?

Copy link
Member Author

@bicknellr bicknellr Sep 5, 2019

Choose a reason for hiding this comment

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

Doesn't seem like it:

src/lit-element.ts:241:23 - error TS2355: A function whose declared type is neither 'void' nor 'any' must return a value.

241   protected render(): unknown|void {
                          ~~~~~~~~~~~~

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a shame to add non-observable, non-type annotation code to satisfy the type-checker. Can we cast our way out of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how I would cast the function in place since class syntax isn't expression based (and I assume we don't want to bind it / remove it from the prototype by making it a property), so I tried pulling it out of the class as this:

LitElement.prototype.render = function() {} as () => unknown;

But then I get another couple of errors that seem like TS is no longer recognizing that render is also on the prototype:

src/lit-element.ts:219:18 - error TS2576: Property 'render' is a static member of type 'LitElement'

219             this.render(),
                     ~~~~~~

src/lit-element.ts:243:22 - error TS2576: Property 'render' is a static member of type 'LitElement'

243 LitElement.prototype.render = function() {} as () => unknown;
                         ~~~~~~

I'm not familiar with pre-classes TS, so maybe that's not the right way to do it?

Copy link
Member Author

Choose a reason for hiding this comment

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

(from offline) I'm going to leave the return undefined; as is and assume that any given minifier should be smart enough to remove it.

@@ -798,6 +798,62 @@ suite('Static get styles', () => {
const bodyStyles = `${cssModule}`;
assert.equal(bodyStyles, '.my-module { color: yellow; }');
});

test.only(
Copy link
Contributor

Choose a reason for hiding this comment

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

remove .only

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@bicknellr
Copy link
Member Author

bicknellr commented Sep 9, 2019

The benchmarks check is stuck (it's been in the 'queued' state all weekend) but I don't know how to restart it. (I already tried clicking "Re-run all checks" under "Tachometer Benchmarks" in the Checks tab.)

@darionco
Copy link

darionco commented Dec 5, 2019

#618 is extremely serious since it affects FireFox and Safari (and others), that's over 20% of the total market share!

Please expedite this PR!!!

@bicknellr bicknellr mentioned this pull request Jan 16, 2020
dfreedm added a commit that referenced this pull request Mar 6, 2020
- #712 Introduced a breaking behavior change for situations where
  `render` is unimplemented, and DOM is added before being connected to
  the document.
dfreedm added a commit that referenced this pull request Mar 7, 2020
)

* Only call lit-html render if LitElement subclass implements render

- #712 Introduced a breaking behavior change for situations where
  `render` is unimplemented, and DOM is added before being connected to
  the document.

* update CHANGELOG

* Remove `connectedCallback` in render test
@bschlenk
Copy link
Contributor

bschlenk commented May 6, 2020

Isn't this a breaking change? My team was not returning anything from render in our non-shadow-dom components that accept children, which would previously prevent render from happening. Now, it blows away all children of the component. I figured out a workaround, which is to return super.render(), but this isn't documented anywhere.

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.

Styles don't get added if render() returns undefined at point of attach to DOM Allow empty template results?
6 participants