-
Notifications
You must be signed in to change notification settings - Fork 879
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
[lit-ssr] Initial commit of lit-ssr package #1347
Conversation
114aaac
to
e939aff
Compare
📊 Tachometer Benchmark ResultsSummary
nop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
⏱ lit-element-stub1lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
updating-element-list
render
update
update-reflect
|
6453779
to
4c586dc
Compare
5aade07
to
5644c5a
Compare
Fix lit-element import paths For now, build template-shadowroot as part of lit-ssr Add lit-ssr tests to CI Fix merge error (Template:_options) Remove tsconfig.tsbuildinfo Uncomment element tests ffrom server-only suite Fix return value of createRenderRoot patch
Rename $litPrivate -> $private Fix demo hydration README updates
5644c5a
to
b3c0d39
Compare
Some minor cleanup included
}); | ||
// There will be two comments per part (open+close) in the rendered | ||
// output and on the client, so increment again for that | ||
// nodeIndex++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to confirm why this is no longer true, but likely due to the client-side change to only make one marker for node parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
packages/lit-html/src/lit-html.ts
Outdated
abstract render(...props: Array<unknown>): unknown; | ||
update(_part: Part, props: Array<unknown>): unknown { | ||
return this.render(...props); | ||
} | ||
} | ||
|
||
const getTemplateHtml = (strings: TemplateStringsArray, type: ResultType) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just broken out of the constructor, for use by render-lit-html
.
@@ -319,15 +322,156 @@ const walker = d.createTreeWalker( | |||
* change in future pre-releases. | |||
*/ | |||
export abstract class Directive { | |||
resolve(part: Part, props: Array<unknown>): unknown { | |||
return this.update(part, props); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to reuse the non-trivial AttributePart
logic for running directives on all of the parts and concatenating everything together, I factored _resolveValue
out of _setValue
, so that we can call that during hydrate and on the server without committing the value to the DOM. However, as part of that we need to do the server thing with directives, aka run its render
not its update
. In order to reuse the AttributePart
logic, I made Directive
have a patchable resolve
method that defaults to running update
but which can be patched by SSR to run render
instead. We can't just patch update
because the user overrides this method. This is maybe one of the more questionable changes. TBD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to _resolve
and make private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (it's @internal
, not private)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without having thought too hard about his yet, I'd be a little inclined to see if we can vary the callsite for directive.update()
instead.
packages/lit-html/src/lit-html.ts
Outdated
/** @internal */ | ||
_setEndNode(node: ChildNode | null) { | ||
this._endNode = node; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During hydration, we need to construct a NodePart
before we've reached its end marker; for now I've added a _setEndNode
method rather than make _endNode
public; TBD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think _startNode
and _endNode
should just be @internal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we do a rollup build for SSR (which we discussed in chat and decided to go the private-support approach for top-level exports), any mangled fields of those members still hit the issue, and need to be in the reserved list (not the cross-package mangle list). So they're both not hidden and not mangled. It's just a choice we need to decide on and then be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I take that back, _startNode/_endNode are only used in hydrate, which can be mangled, so I'll go for that.
"description": "", | ||
"main": "index.js", | ||
"scripts": { | ||
"build": "(cd node_modules/template-shadowroot && npm run build) && tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'm depending on the github repo for template-shadowroot
and building it package from source as part of the lit-ssr build
script since it's not released. We should discuss whether to publish a release yet or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless something has changed recently with the spec'd feature that requires the package to be updated, I think we should release it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We can do a pre-release version for now.
Co-authored-by: Abdón Rodríguez Davila <a@abdonrd.com>
Return `noChage` from default render now that we don't clear the container before rendering, which was the reason for renderNotImplemented.
@justinfagnani @sorvell I think I addressed most of the feedback in here, PTAL. |
new (...args: any[]): PatchableLitElement; | ||
createRenderRoot(): Element | ShadowRoot; | ||
_needsHydration: boolean; | ||
_renderImpl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
(container as any).$lit$ = rootPart; | ||
}; | ||
|
||
const openNodePart = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain that after calling hydrate lit-html will behave as if it just initially rendered that DOM and any subsequent renders will update efficiently, the same as if lit-html had rendered the DOM on the client.
- name: Lint | ||
run: npm run lint | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: Lint | |
run: npm run lint |
Now lit-next
branch already has a lint step! 🙂
@@ -139,10 +130,14 @@ export class LitElement extends UpdatingElement { | |||
* the element to update. | |||
*/ | |||
protected render(): unknown { | |||
return renderNotImplemented; | |||
return noChange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this an intended change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, that deserves some discussion. @sorvell had suggested just re-implementing LitElement
's update
in hydrate-support rather than adding a patchable _renderImpl
, since LitElement's update
is pretty small, and that would have required exporting the renderNotImplemented
symbol. But in thinking about why we had that, it was to disambiguate whether the user implemented render so that we didn't clear the light DOM if the user had not implemented render
. Since we don't clear the container now, I don't think we need to treat "not implemented" special anymore, we can just return noChange
to not do anything in render
. Seems better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this was the change where renderNotImplemented
was introduced: lit/lit-element#917:
lit/lit-element#712 Introduced a breaking behavior change for situations where
render is unimplemented, and DOM is added before being connected to
the document.
packages/lit-html/src/lit-html.ts
Outdated
@@ -35,7 +35,7 @@ const markerMatch = '?' + marker; | |||
// syntax because it's slightly smaller, but parses as a comment node. | |||
const nodeMarker = `<${markerMatch}>`; | |||
|
|||
const d = document; | |||
const d = window.document; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be necessary since we patch the global. Is this for the non-importModule use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we don't actually add the document
instance to the shim. Lemme do that.
packages/lit-html/src/lit-html.ts
Outdated
); | ||
const walker = | ||
d && | ||
d.createTreeWalker( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d?.createWalker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (removed, added createTreeWalker stub to shim)
@@ -319,15 +322,156 @@ const walker = d.createTreeWalker( | |||
* change in future pre-releases. | |||
*/ | |||
export abstract class Directive { | |||
resolve(part: Part, props: Array<unknown>): unknown { | |||
return this.update(part, props); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without having thought too hard about his yet, I'd be a little inclined to see if we can vary the callsite for directive.update()
instead.
packages/lit-html/src/lit-html.ts
Outdated
return { | ||
// We don't technically need to close the SVG tag since the parser | ||
// will handle it for us, but the SSR parser doesn't like that | ||
html: html + strings[l] + (type === SVG_RESULT ? '</svg>' : ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these names be minified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we don't have a great way to minify object/class fields that are used in SSR (just top-level exports via the $private
export). Since this is a very private API, seems chill enough to return an array ([html, attrNames]
), so I'll do that.
@@ -751,7 +784,7 @@ export class NodePart { | |||
// TODO (justinfagnani): To support nested directives, we'd need to | |||
// resolve the directive result's values. We may want to offer another | |||
// way of composing directives. | |||
this._setValue(this._directive.update(this, value.values)); | |||
this._setValue(this._directive._resolve(this, value.values)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much benefit do you see out of reusing the part classes in their entirety? If we moved some of the methods to functions, could we use them to build SSR-specific implementations that call directive.render()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in SSR we only construct AttributePart
(the code to run the directive for NodePart
is chill enough that I just copied it). For AttributePart
, we basically want to reuse _resolveValue
, which sequentially resolves each directive, deals with the nothing/noChange, and then concatenates the new string. Even if we made that into a function, we'd still need to thread a flag or function through to do something different on the server. Not sure that's worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ok, so I've confirmed that any We'll look into optimizing directives in a separate issue: #1398. |
@@ -359,12 +357,11 @@ const createAttributeParts = ( | |||
// directive value; we only then commit that value for event/property | |||
// parts since those were not serialized (passing `undefined` uses the | |||
// default commitValue; passing `noOpCommit` bypasses it) | |||
const commitValue = | |||
const noCommit = !( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comment above this.
Port of SSR implementation from proof-off-concept work in
lit-ssr
repo,lit-html#hydration
branch, andlit-element#hydration
branch.Current status:
render-lit-html
tests passinglit-html
integration tests passingLitElement
integration tests passingrenderLight
directive implemented / tests passingNote that the work under #1360 (additional fixes/features for end-to-end app proof-of concept) will be done in a follow-on PR.
Current size comparison: (as of b3c0d39)
Notes for reviewers:
The first commit was a copy/paste of
lit-ssr
andhydrate.ts
from the POC repo/branches, so https://github.com/Polymer/lit-html/compare/938459a..b3c0d39 will compare from that baseline forward to just before the formatting changes added at the end that make it much more difficult to difflit-html
export const $private = { ... }
export tolit-html
which exports private top-level members needed by hydrate & render-lit-html as mangled names, which both keeps file size down and reinforces private nature of themprivate-ssr-support.js
module which re-exports the fields in$private
back to well-known symbol names for use inlit-ssr
, so that package is agnostic to whether the lit packages are in dev or prod moderenderLight
under lit-html/directives
; we had this inLitElement
, but it's not reallyLitElement
-specific per se, and it fits better under lit-html directives than as a one-off underLitElement
NodePart
before we've reached its end marker; for now I've added a_setEndNode
method rather than make_endNode
public; TBDAttributePart
logic for running directives on all of the parts and concatenating everything together, I factored_resolveValue
out of_setValue
, so that we can call that during hydrate and on the server without committing the value to the DOM. However, as part of that we need to do the server thing with directives, aka run itsrender
not itsupdate
. In order to reuse theAttributePart
logic, I madeDirective
have a patchableresolve
method that defaults to runningupdate
but which can be patched by SSR to runrender
instead. This is maybe one of the more questionable changes. TBDlit-element
hydrate-support.js
module which installs thehydrate
codepath into LitElement_renderImpl
toLitElement
as a patching point forhydrate-support
(similar to the oldstatic render
method, but internal and on the prototype)template-shadowroot
build
script since it's not released. We should discuss whether to publish a release yet or not.lit-ssr
lit-ssr
from a branch containing following PR's which were technically not yet merged intolit-ssr
, so it's worth scrutinizing these changes also: