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

[SSR/Core] Add @lit-labs/ssr-dom-shim package #3522

Merged
merged 51 commits into from
Jan 6, 2023
Merged

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Dec 14, 2022

  • Adds a new @lit-labs/ssr-dom-shim package which exports Element, HTMLElement, CustomElementRegistry, and a default global customElements (but does not register any of them as globals).

  • Updates @lit/reactive-element/reactive-element.js to import from this package in its node build, instead of providing inline no-op versions. This means the parts of the DOM shim required for most Lit components will always be loaded in Node, so that users don't need to think about loading the DOM shim at all anymore.

  • When using this new automatic shimming by importing reactive-element from Node, only a single global will now be defined: customElements. So in particular, window will not be defined, solving many issues we encountered with libraries sniffing for window to switch between node vs browser modes.

  • ModuleLoader now provides a default VM global context object which provides basic globals that are available in both Node and browsers. A context object can also be directly created with a new makeDefaultContextObject function.

  • Updates lit-html/experimental-hydrate.js to not depend on btoa when running in Node, instead using Node's built-in Buffer.

  • Updates the existing @lit-labs/ssr/lib/dom-shim.js module to import HTMLElement and customElements from the new package, so that the same base class and registry are used when the full DOM shim is loaded.

  • Makes registering a duplicate element in the DOM shim throw an exception.

  • Misc minor improvements to the DOM shim code, including moving APIs from HTMLElement to Element, which is the correct interface for them.

  • Updates to the following packages to remove usage of the global DOM shim in code/tests/READMEs:

    • @lit-labs/testing
    • @lit-labs/eleventy-plugin-lit
    • @lit-labs/ssr
    • @lit/localize-tools

See also lit/lit.dev#1026 which updates the lit.dev docs for these changes.

Fixes #3523

@changeset-bot
Copy link

changeset-bot bot commented Dec 14, 2022

🦋 Changeset detected

Latest commit: 4f01ca2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@lit-labs/eleventy-plugin-lit Major
@lit-labs/ssr Major
@lit-labs/testing Minor
@lit-labs/ssr-dom-shim Major
@lit/reactive-element Minor
lit-html Minor
lit Minor

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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 14, 2022

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -4% - +1% (-0.97ms - +0.25ms)
    this-change vs tip-of-tree

render

  • lit-element-list: 81.03ms - 83.52ms
  • lit-html-kitchen-sink: unsure 🔍 -6% - +0% (-1.81ms - +0.14ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -6% - +4% (-0.66ms - +0.48ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -2% - +3% (-0.83ms - +1.38ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -1% - +5% (-0.43ms - +2.70ms)
    this-change vs tip-of-tree

update

  • lit-element-list: 741.87ms - 751.58ms
  • lit-html-kitchen-sink: unsure 🔍 -3% - +6% (-2.13ms - +4.73ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -10% - +9% (-31.15ms - +27.94ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -1% - +2% (-1.39ms - +1.92ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -1% - +1% (-8.88ms - +4.39ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: 743.47ms - 751.53ms
  • reactive-element-list: unsure 🔍 -1% - +1% (-10.52ms - +4.23ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs
81.03ms - 83.52ms-

update

VersionAvg timevs
741.87ms - 751.58ms-

update-reflect

VersionAvg timevs
743.47ms - 751.53ms-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
30.99ms - 31.80ms-unsure 🔍
-6% - +0%
-1.81ms - +0.14ms
unsure 🔍
-6% - +1%
-1.89ms - +0.38ms
tip-of-tree
tip-of-tree
31.35ms - 33.12msunsure 🔍
-0% - +6%
-0.14ms - +1.81ms
-unsure 🔍
-4% - +5%
-1.30ms - +1.46ms
previous-release
previous-release
31.09ms - 33.21msunsure 🔍
-1% - +6%
-0.38ms - +1.89ms
unsure 🔍
-5% - +4%
-1.46ms - +1.30ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
80.76ms - 85.32ms-unsure 🔍
-3% - +6%
-2.13ms - +4.73ms
unsure 🔍
-6% - +2%
-5.20ms - +1.70ms
tip-of-tree
tip-of-tree
79.18ms - 84.30msunsure 🔍
-6% - +3%
-4.73ms - +2.13ms
-unsure 🔍
-8% - +1%
-6.69ms - +0.58ms
previous-release
previous-release
82.21ms - 87.37msunsure 🔍
-2% - +6%
-1.70ms - +5.20ms
unsure 🔍
-1% - +8%
-0.58ms - +6.69ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
21.97ms - 22.75ms-unsure 🔍
-4% - +1%
-0.97ms - +0.25ms
unsure 🔍
-5% - +2%
-1.08ms - +0.54ms
tip-of-tree
tip-of-tree
22.25ms - 23.18msunsure 🔍
-1% - +4%
-0.25ms - +0.97ms
-unsure 🔍
-3% - +4%
-0.76ms - +0.94ms
previous-release
previous-release
21.92ms - 23.33msunsure 🔍
-2% - +5%
-0.54ms - +1.08ms
unsure 🔍
-4% - +3%
-0.94ms - +0.76ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.38ms - 12.19ms-unsure 🔍
-6% - +4%
-0.66ms - +0.48ms
unsure 🔍
-3% - +8%
-0.34ms - +0.88ms
tip-of-tree
tip-of-tree
11.47ms - 12.28msunsure 🔍
-4% - +6%
-0.48ms - +0.66ms
-unsure 🔍
-2% - +9%
-0.25ms - +0.97ms
previous-release
previous-release
11.05ms - 11.97msunsure 🔍
-7% - +3%
-0.88ms - +0.34ms
unsure 🔍
-8% - +2%
-0.97ms - +0.25ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
298.87ms - 342.43ms-unsure 🔍
-10% - +9%
-31.15ms - +27.94ms
unsure 🔍
-10% - +8%
-33.00ms - +26.63ms
tip-of-tree
tip-of-tree
302.29ms - 342.22msunsure 🔍
-9% - +10%
-27.94ms - +31.15ms
-unsure 🔍
-9% - +8%
-30.09ms - +26.93ms
previous-release
previous-release
303.48ms - 344.19msunsure 🔍
-8% - +10%
-26.63ms - +33.00ms
unsure 🔍
-8% - +9%
-26.93ms - +30.09ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
54.08ms - 55.62ms-unsure 🔍
-2% - +3%
-0.83ms - +1.38ms
slower ❌
0% - 4%
0.11ms - 2.03ms
tip-of-tree
tip-of-tree
53.79ms - 55.37msunsure 🔍
-3% - +2%
-1.38ms - +0.83ms
-unsure 🔍
-0% - +3%
-0.17ms - +1.77ms
previous-release
previous-release
53.21ms - 54.35msfaster ✔
0% - 4%
0.11ms - 2.03ms
unsure 🔍
-3% - +0%
-1.77ms - +0.17ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
110.34ms - 112.51ms-unsure 🔍
-1% - +2%
-1.39ms - +1.92ms
unsure 🔍
-1% - +2%
-0.82ms - +2.13ms
tip-of-tree
tip-of-tree
109.91ms - 112.40msunsure 🔍
-2% - +1%
-1.92ms - +1.39ms
-unsure 🔍
-1% - +2%
-1.21ms - +1.98ms
previous-release
previous-release
109.77ms - 111.77msunsure 🔍
-2% - +1%
-2.13ms - +0.82ms
unsure 🔍
-2% - +1%
-1.98ms - +1.21ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
53.38ms - 55.49ms-unsure 🔍
-1% - +5%
-0.43ms - +2.70ms
unsure 🔍
-5% - +2%
-2.91ms - +1.00ms
tip-of-tree
tip-of-tree
52.14ms - 54.46msunsure 🔍
-5% - +1%
-2.70ms - +0.43ms
-faster ✔
0% - 7%
0.07ms - 4.10ms
previous-release
previous-release
53.74ms - 57.03msunsure 🔍
-2% - +5%
-1.00ms - +2.91ms
slower ❌
0% - 8%
0.07ms - 4.10ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
750.47ms - 758.93ms-unsure 🔍
-1% - +1%
-8.88ms - +4.39ms
unsure 🔍
-1% - +1%
-8.66ms - +4.61ms
tip-of-tree
tip-of-tree
751.82ms - 762.06msunsure 🔍
-1% - +1%
-4.39ms - +8.88ms
-unsure 🔍
-1% - +1%
-7.01ms - +7.46ms
previous-release
previous-release
751.60ms - 761.83msunsure 🔍
-1% - +1%
-4.61ms - +8.66ms
unsure 🔍
-1% - +1%
-7.46ms - +7.01ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
771.40ms - 781.76ms-unsure 🔍
-1% - +1%
-10.52ms - +4.23ms
unsure 🔍
-1% - +1%
-4.64ms - +9.09ms
tip-of-tree
tip-of-tree
774.48ms - 784.97msunsure 🔍
-1% - +1%
-4.23ms - +10.52ms
-unsure 🔍
-0% - +2%
-1.54ms - +12.28ms
previous-release
previous-release
769.86ms - 778.86msunsure 🔍
-1% - +1%
-9.09ms - +4.64ms
unsure 🔍
-2% - +0%
-12.28ms - +1.54ms
-

tachometer-reporter-action v2 for Benchmarks

Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

This is great! Some comments below.

packages/labs/ssr-dom-shim/package.json Outdated Show resolved Hide resolved
packages/labs/ssr-dom-shim/package.json Show resolved Hide resolved
packages/labs/ssr-dom-shim/src/index.ts Show resolved Hide resolved
rollup-common.js Outdated Show resolved Hide resolved
Copy link
Member Author

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Comments addressed.

Also addressed two comments from @kevinpschaaf that were made in an in-person conversation:

  • reactive-element.js in Node now falls back to globalThis.HTMLElement if one has been defined, which matches the behavior we had before, and allows users to use alternative DOM shims if they want.

  • The existing getWindow function from the SSR package now behaves like it did before, in that it returns a new CustomElementRegistry for each object, instead of using a global singleton. This is important for VM modules that are using getWindow, since otherwise there would be a shared CE registry between different VM render contexts.

rollup-common.js Outdated Show resolved Hide resolved
packages/labs/ssr-dom-shim/src/index.ts Show resolved Hide resolved
packages/labs/ssr-dom-shim/package.json Show resolved Hide resolved
packages/labs/ssr-dom-shim/package.json Outdated Show resolved Hide resolved
@aomarks aomarks marked this pull request as ready for review December 15, 2022 00:18
@aomarks
Copy link
Member Author

aomarks commented Dec 15, 2022

Ah, seeing failures relating to code that looks at the TypeScript AST. Now I remember why we did the weird temporary-global-assignment trick previously. Though that won't work here, because of the shadowing we have due to the import. Will need to investigate another pattern.

   FAIL  legacy class field emit  "@localized"
    Expected values to be deeply equal:  (equal)

        ··[
        Actual:
        --··"../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",
        ··]

@aomarks
Copy link
Member Author

aomarks commented Jan 6, 2023

Also just pushed updates to the following packages to remove usage of the global DOM shim in its code/tests/READMEs:

  • @lit-labs/testing (the actual code)
  • @lit-labs/eleventy-plugin-lit (the actual code)
  • @lit-labs/ssr (the README)
  • @lit/localize-tools (a test)

There is now no usage of the global DOM shim in any of our packages.

@aomarks
Copy link
Member Author

aomarks commented Jan 6, 2023

See also lit/lit.dev#1026 which updates the lit.dev docs to account for this new approach of automatic shimming.

Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

awesome!

Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

This is fantastic! Super excited to see this land.

Minor suggestions for importing render from @lit-labs/ssr in other packages.

packages/labs/eleventy-plugin-lit/src/worker/worker.ts Outdated Show resolved Hide resolved
packages/labs/testing/src/lib/worker.ts Outdated Show resolved Hide resolved
packages/localize-tools/CHANGELOG.md Outdated Show resolved Hide resolved
packages/localize-tools/src/tests/ssr.unit.test.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
---
'@lit-labs/eleventy-plugin-lit': minor
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if this change should be major since it's potentially removing previously shimmed globals?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it should be, but since we're still on 0.x.x we have to call it minor or else changesets will release 1.0.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although now that I think of it, let's just release 1.0.0

@aomarks aomarks force-pushed the dom-shim-package branch 2 times, most recently from 50182cf to 14b0d94 Compare January 6, 2023 22:49
@aomarks aomarks enabled auto-merge (squash) January 6, 2023 23:10
@aomarks aomarks merged commit 72fcf0d into main Jan 6, 2023
@aomarks aomarks deleted the dom-shim-package branch January 6, 2023 23:11
@lit-robot lit-robot mentioned this pull request Jan 9, 2023
aomarks added a commit that referenced this pull request Jan 9, 2023
The automatic merge that git did between #3522 and #3549 resulted in a bad `env` config. This was causing the SSR tests to fail, since the `--experimental-vm-modules` flag stopped getting passed.
@lit-robot lit-robot mentioned this pull request Jan 9, 2023
aomarks added a commit to lit/lit.dev that referenced this pull request Jan 9, 2023
This updates the lit.dev SSR documentation to account for the changes in lit/lit#3522, namely the removal of the global DOM shim.
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.

[SSR] Bake DOM shim into reactive-element
4 participants