[core/ssr] Add Node builds that don't explode #3156
Conversation
🦋 Changeset detectedLatest commit: 8367869 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
rollup-common.js
Outdated
| @@ -255,6 +255,7 @@ export function litProdConfig({ | |||
| packageName, | |||
| outputDir = './', | |||
| copyHtmlTests = true, | |||
| nodeBuild = false, | |||
There was a problem hiding this comment.
Nit: naming, maybe includeNodeBuild? (so it doesn't sound like you're configuring the build to be for node specifically)
|
|
||
| if (NODE_MODE) { | ||
| global.HTMLElement ??= class HTMLElement {} as unknown as typeof HTMLElement; | ||
| global.customElements ??= { |
There was a problem hiding this comment.
All things equal, if a user didn't explicitly load the dom-shim, it'd be nice to keep as much as possible off the global. I think we could pull HTMLElement into a local variable (and shim it there) rather than put it on the global, right?
customElements is a little harder; we could do the same in the decorator, and that would just leave JS users to needing to do customElements?.define(...) in their own elements. But maybe it's worth keeping customElements on the global to help smooth that over?
There was a problem hiding this comment.
Agreed. I think if this approach is good it's exactly because it 1) puts all the cost on the node import, so 2) can go to extra lengths to isolate any shims to just this module.
I think we should be able to do something like:
const HTMLElementShim = class HTMLElement { ... }
export class ReactiveElement extends (NODE ? HTMLElementShim : HTMLElement) {Regarding window.customElements, shouldn't that be shimmed, if at all, in the decorator definition module? And there we just need the decorator to not access it, yeah?
There was a problem hiding this comment.
Let's sync on this offline. It seems like Al is moving towards not having a DOM shim separate from 'lit is loaded in Node'? If that' the case, we do want to populate the global in the Node import condition, yeah?
There was a problem hiding this comment.
As per meeting this morning, HTMLElement is no longer globally shimmed, it is instead just declared inline. The reason we don't need HTMLElement globally is that we're only concerned with users who are extending ReactiveElement or LitElement.
I have kept the global customElements definitions so that it will be possible to import components that aren't defined using the decorator.
There was a problem hiding this comment.
Just pushed another change that shims HTMLElement in a slightly more sneaky way. It now temporarily defines globalThis.HTMLElement, and then removes it after ReactiveElement is defined, instead of changing the extends clause at all.
The reason for this is that I found that the following code ... :
export abstract class ReactiveElement
extends (NODE_MODE && global.HTMLElement === undefined
? (class HTMLElement {} as unknown as typeof HTMLElement)
: HTMLElement)... resulted in in this d.ts output:
declare const ReactiveElement_base: {
new (): HTMLElement;
prototype: HTMLElement;
};
/**
* Base element class which manages element properties and attributes. When
* properties change, the `update` method is asynchronously called. This method
* should be supplied by subclassers to render updates as desired.
* @noInheritDoc
*/
export declare abstract class ReactiveElement extends ReactiveElement_base implements ReactiveControllerHost {Which, for some reason, causes ts-transformers to fail with this error:
--··"../reactive-element/reactive-element.d.ts(181,55):·error·TS2749:·'ReactiveElement_base'·refers·to·a·value,·but·is·being·used·as·a·type·here.·Did·you·mean·'typeof·ReactiveElement_base'?\n",
I also have a suspicion that the ternary extends could be upsetting to Closure compiler.
Open to other suggestions, but this approach of defining and then undefining it should be the least confusing to type systems.
There was a problem hiding this comment.
I can't think of any better way. I tried playing around with it a bit but TS also did not like an exported class extending something that's module scoped.
| import {LitElement, html} from 'lit-element'; | ||
| import {customElement} from 'lit-element/decorators.js'; | ||
|
|
||
| @customElement('my-element') |
There was a problem hiding this comment.
Make sense to include a version that doesn't use the decorator as well?
| /** | ||
| * A tiny module scoped polyfill for HTMLSlotElement.assignedElements. | ||
| */ | ||
| const slotAssignedElements = | ||
| window.HTMLSlotElement?.prototype.assignedElements != null | ||
| global.HTMLSlotElement?.prototype.assignedElements != null |
There was a problem hiding this comment.
Is HTMLSlotElement going to be defined in Node? Is that in the dom-shim? And what about all the other element classes? Users probably won't use ?. for these as is done here, right?
There was a problem hiding this comment.
No, and the broader DOM shim doesn't define HTMLSlotElement either. Somebody could be doing e.g. instanceof HTMLSlotElement somewhere, but I think we don't need to worry about it for now.
| import {customElement} from '@lit/reactive-element/decorators.js'; | ||
| import {ReactiveElement} from '@lit/reactive-element'; | ||
|
|
||
| @customElement('my-element') |
There was a problem hiding this comment.
Add a version that doesn't use the decorator?
|
|
||
| if (NODE_MODE) { | ||
| global.HTMLElement ??= class HTMLElement {} as unknown as typeof HTMLElement; | ||
| global.customElements ??= { |
There was a problem hiding this comment.
Let's sync on this offline. It seems like Al is moving towards not having a DOM shim separate from 'lit is loaded in Node'? If that' the case, we do want to populate the global in the Node import condition, yeah?
|
Just added a fix to the |
| ); | ||
| let replaced = false; | ||
| const newSource = fileSource.replace(versionVarRegex, () => { | ||
| const newSource = fileSource.replace(versionVarRegex, (_, pre, post) => { |
There was a problem hiding this comment.
Ooh very nice utilization of capture groups.
There was a problem hiding this comment.
Looks good.
Since we have to commit to this approach long-term, I'm mildly interested in how much code it would add if we just did the shimming in the normal codepath (and not have a separate node export condition and all the config file overhead that requires).
But since as you note the node export condition option will also give us a nice place to put the isSSR flag, seems like that having a NODE_MODE will end up being useful for a number of reasons, so I think I'm good with this direction.
Below is a comparison of the sizes of the two main affected files. "Prod" is the minified browser build (
So, a little more substantial on the Also I believe this difference will increase even further, because I plan to extend the Node |
|
Cool thanks. The 90-100b on RE is definitely worth it, plus what you said about possibly expanding it a bit, so I think I'm sold. |
Background
Before this PR, importing Lit with Node causes errors such as
window is not defined. The only way to load Lit was to first ensure that thedom-shim.jsmodule provided by the@lit-labs/ssrpackage was loaded, which defineswindowplus APIs likeHTMLElementandcustomElements.This meant that using Lit in frameworks such as Next did not work out-of-the-box even when only client-side rendering is needed.
This change
After this PR, Lit can be imported from Node without errors, and custom elements can be defined as no-ops.
This doesn't remove the need for loading the DOM shim shim when SSR is actually needed, but it does give us compatibility at least for client-side rendering in frameworks like Next, without the user needing to do anything special.
This works by adding a Node-specific Rollup build and a
nodeexport condition tolit-htmland@lit/reactive-element. Neitherlit-elementnorlitneed a Node build, because only the underlying 2 libraries currently need Node-specific behavior.This only defines
customElementsas new globals. It does not definewindow,HTMLElement, or any other APIs. This way, we won't affect the behavior of libraries that detect whether they are in Node vs the browser by checking for awindowglobal.For testing, I've added a
node-imports.tsfile to each package, and added a newnode:testscript which executes that module with Node. This confirms that Node doesn't crash on import.Fixes #3079
Followup
In a follow up, I will propose moving the actual functional dom-shim implementations of
HTMLElementandcustomElementsfrom@lit-labs/ssrintoreactive-element(in this PR they are just no-ops) so that ultimately the user never has to load the dom-shim manually.However, in order to do this, we will need to update the Lit SSR
ModuleLoaderto understand export conditions -- because currently it does not support them, so it will not find the Node build. We depend on theresolvepackage to do module resolution, which unfortunately does not yet support export conditions, so this work will be a little bit non-trivial.Another followup will be to add an
isSsrconst or function. Rather than doing runtime detection, this can simply betruein the node build andfalsein the browser build.Size
This actually slightly drops the size of the main
lit-html.jsandreactive-element.jsproduction build files: