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

Make the custom element instance responsible for rendering. #65

Closed
wants to merge 3 commits into from

Conversation

kevinpschaaf
Copy link
Contributor

@kevinpschaaf kevinpschaaf commented Jul 2, 2020

  • Adds minimal attribute API to the dom-shim
  • custom-element opcodes refactored to call setAttribute for static attributes, connectedCallback after completing all bindings (which set properties/attributes directly on the instance), and then render the children based on a new ssrRenderChildren field expected to be set on the CE instance by the time connectedCallback returns
  • This eliminates the need for an explicit ElementRenderer class, allowing us to reuse as much logic as possible from the CE base class
  • lit-element-renderer.ts is now responsible for patching LitElement.render to set a children-rendering generator onto instance.ssrRenderChildren and to patch connectedCallback to synchronously call performUpdate().
  • Note that LitElement:performUpdate() is called with a new flag to elide the (first)updated callbacks, since these (a) can't do anything good to the SSR output since they occur after render(), and (b) may more likely touch DOM.
  • Adds SSRRenderOptions to render(), and if deferChildHydration is true, it outputs a defer-hydration attribute on each CE
  • Escapes all attribute & textContent values (resolves TODO in code)

* Adds minimal attribute API to the dom-shim
* custom-element opcodes refactored to call setAttribute for static attributes, connectedCallback after completing all bindings (which set properties/attributes directly on the instance), and then render the children based on a new `ssrRenderChildren` field expected to be set on the CE instance by the time `connectedCallback` returns
* This eliminates the need for an explicit `ElementRenderer` class, allowing us to reuse as much logic as possible from the CE base class
* `lit-element-renderer.ts` is now responsible for patching `LitElement.render` to set a children-rendering generator onto `instance.ssrRenderChildren` and to patch `connectedCallback` to synchronously call `performUpdate()`.
* Note that `LitElement:performUpdate()` is called with a new flag to elide the `(first)updated` callbacks, since these (a) can't do anything good to the SSR output since they occur after `render()`, and (b) may more likely touch DOM.
};

/**
* Operation to render a custom element, usually its shadow root.
*/
type CustomElementRenderOp = {
type: 'custom-element-render';
type CustomElementRenderChildrenOp = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Update doc and add one below. Why are they separate? Is it to allow the instance to reflect attributes before streaming them?

const instance = getLast(renderInfo.customElementInstanceStack);
if (instance !== undefined) {
yield* instance.renderElement();
if (instance.connectedCallback) {
instance?.connectedCallback();
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to more strictly define the server-side lifecycle, but right now I feel strange about calling connectedCallback on the server. That seems like it could be a good signal that the DOM is available. Can we call the methods that it calls directly?

const instance = getLast(renderInfo.customElementInstanceStack);
if (instance !== undefined) {
yield* instance.renderElement();
if (instance.connectedCallback) {
instance?.connectedCallback();
Copy link
Contributor

Choose a reason for hiding this comment

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

instance?. <- the optional chaining should be unnecessary inside the if (instance !== undefined) { check

type RenderableCustomElement = HTMLElement & {
new(): RenderableCustomElement;
connectedCallback?(): void;
ssrRenderChildren?: IterableIterator<string> | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this. Seems like it's replacing the ElementRenderer with more responsibility on the instance, but I do like the separation of concerns with the server-only ElementRenderer interface.

@@ -384,8 +409,15 @@ const getTemplate = (result: TemplateResult) => {
return t;
};

type RenderableCustomElement = HTMLElement & {
Copy link
Contributor

Choose a reason for hiding this comment

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

an interface that extends HTMLElement will let you remove the new() declaration.

@kevinpschaaf
Copy link
Contributor Author

Closing, in favor of #66

@kevinpschaaf kevinpschaaf deleted the element-as-renderer branch July 7, 2020 22:17
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

Successfully merging this pull request may close these issues.

2 participants