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-client] Move experimental hydrate modules #3720

Merged
merged 10 commits into from Apr 3, 2023

Conversation

augustjk
Copy link
Member

@augustjk augustjk commented Mar 10, 2023

Closes #3713

This PR can be reviewed commit by commit.

  1. Copy experimental hydrate modules to ssr-client
    Copies contents of experimental files to new location
  2. Update to fix type errors after copying
    As _$LH is being pulled from lit-html package, @internal marked properties became invisible.
    Some of the @internal markings for classes and types that were never meant for public consumption were removed.
    For others, like those in ChildPart and AttributePart, type assertions were added.
  3. Update ssr-client package
    Update package.json files, exports, wireit configs, rollup to add new entry and node build.
  4. Add deprecation notes to original
    I'm leaving the original contents there and marking deprecated, since trying to re-export from @lit-labs/ssr-client introduces a circular dependency as @lit-labs/ssr-client itself depends on lit-html.
    experimental-hydrate-support.js is side-effectful so added a console.warn for deprecation warning.

To do: maybe in a separate PR, would be to update all our docs, examples, and integrations to use the new location.

Open to discussion: if we need to make any changes to these going forward, do we update both locations until we remove the originals from lit-html and lit-element (Lit3?) or just update the @lit-labs/ssr-client?

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2023

🦋 Changeset detected

Latest commit: b992c5d

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

This PR includes changesets to release 9 packages
Name Type
@lit-labs/eleventy-plugin-lit Patch
@lit-labs/ssr-react Patch
@lit-labs/testing Patch
@lit-labs/nextjs Patch
@lit-labs/ssr Patch
@lit-labs/ssr-client Minor
lit-element Patch
lit-html Patch
lit Patch

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 Mar 10, 2023

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -5% - +2% (-0.78ms - +0.35ms)
    this-change vs tip-of-tree

render

  • lit-element-list: 75.81ms - 79.03ms
  • lit-html-kitchen-sink: unsure 🔍 -4% - +8% (-1.46ms - +2.74ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -2% - +11% (-0.22ms - +1.17ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -3% - +1% (-1.78ms - +0.68ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -6% - +0% (-2.98ms - +0.04ms)
    this-change vs tip-of-tree

update

  • lit-element-list: 753.09ms - 762.46ms
  • lit-html-kitchen-sink: unsure 🔍 -3% - +4% (-2.44ms - +2.84ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -9% - +10% (-27.32ms - +32.20ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -2% - +1% (-2.44ms - +1.07ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -1% - +1% (-10.30ms - +8.95ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: 726.25ms - 734.48ms
  • reactive-element-list: unsure 🔍 -1% - +1% (-8.61ms - +6.62ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs
75.81ms - 79.03ms-

update

VersionAvg timevs
753.09ms - 762.46ms-

update-reflect

VersionAvg timevs
726.25ms - 734.48ms-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
32.06ms - 35.04ms-unsure 🔍
-4% - +8%
-1.46ms - +2.74ms
unsure 🔍
-7% - +6%
-2.46ms - +1.92ms
tip-of-tree
tip-of-tree
31.44ms - 34.38msunsure 🔍
-8% - +4%
-2.74ms - +1.46ms
-unsure 🔍
-9% - +4%
-3.09ms - +1.26ms
previous-release
previous-release
32.22ms - 35.43msunsure 🔍
-6% - +7%
-1.92ms - +2.46ms
unsure 🔍
-4% - +9%
-1.26ms - +3.09ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
78.79ms - 82.54ms-unsure 🔍
-3% - +4%
-2.44ms - +2.84ms
unsure 🔍
-5% - +2%
-3.81ms - +1.84ms
tip-of-tree
tip-of-tree
78.61ms - 82.33msunsure 🔍
-4% - +3%
-2.84ms - +2.44ms
-unsure 🔍
-5% - +2%
-3.99ms - +1.62ms
previous-release
previous-release
79.55ms - 83.76msunsure 🔍
-2% - +5%
-1.84ms - +3.81ms
unsure 🔍
-2% - +5%
-1.62ms - +3.99ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
16.48ms - 17.31ms-unsure 🔍
-5% - +2%
-0.78ms - +0.35ms
unsure 🔍
-3% - +4%
-0.49ms - +0.65ms
tip-of-tree
tip-of-tree
16.73ms - 17.49msunsure 🔍
-2% - +5%
-0.35ms - +0.78ms
-unsure 🔍
-1% - +5%
-0.25ms - +0.83ms
previous-release
previous-release
16.43ms - 17.20msunsure 🔍
-4% - +3%
-0.65ms - +0.49ms
unsure 🔍
-5% - +1%
-0.83ms - +0.25ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.05ms - 12.07ms-unsure 🔍
-2% - +11%
-0.22ms - +1.17ms
unsure 🔍
-5% - +7%
-0.54ms - +0.85ms
tip-of-tree
tip-of-tree
10.61ms - 11.56msunsure 🔍
-10% - +2%
-1.17ms - +0.22ms
-unsure 🔍
-9% - +3%
-0.99ms - +0.35ms
previous-release
previous-release
10.93ms - 11.88msunsure 🔍
-7% - +5%
-0.85ms - +0.54ms
unsure 🔍
-3% - +9%
-0.35ms - +0.99ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
295.47ms - 338.69ms-unsure 🔍
-9% - +10%
-27.32ms - +32.20ms
unsure 🔍
-6% - +12%
-17.25ms - +36.52ms
tip-of-tree
tip-of-tree
294.18ms - 335.09msunsure 🔍
-10% - +9%
-32.20ms - +27.32ms
-unsure 🔍
-6% - +11%
-18.77ms - +33.16ms
previous-release
previous-release
291.45ms - 323.43msunsure 🔍
-11% - +5%
-36.52ms - +17.25ms
unsure 🔍
-10% - +6%
-33.16ms - +18.77ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
54.45ms - 56.18ms-unsure 🔍
-3% - +1%
-1.78ms - +0.68ms
unsure 🔍
-2% - +2%
-1.14ms - +1.34ms
tip-of-tree
tip-of-tree
54.99ms - 56.74msunsure 🔍
-1% - +3%
-0.68ms - +1.78ms
-unsure 🔍
-1% - +3%
-0.60ms - +1.90ms
previous-release
previous-release
54.33ms - 56.11msunsure 🔍
-2% - +2%
-1.34ms - +1.14ms
unsure 🔍
-3% - +1%
-1.90ms - +0.60ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
111.42ms - 113.20ms-unsure 🔍
-2% - +1%
-2.44ms - +1.07ms
unsure 🔍
-2% - +1%
-2.10ms - +0.73ms
tip-of-tree
tip-of-tree
111.48ms - 114.50msunsure 🔍
-1% - +2%
-1.07ms - +2.44ms
-unsure 🔍
-2% - +2%
-1.86ms - +1.87ms
previous-release
previous-release
111.89ms - 114.09msunsure 🔍
-1% - +2%
-0.73ms - +2.10ms
unsure 🔍
-2% - +2%
-1.87ms - +1.86ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
49.64ms - 51.57ms-unsure 🔍
-6% - +0%
-2.98ms - +0.04ms
unsure 🔍
-4% - +2%
-1.79ms - +1.23ms
tip-of-tree
tip-of-tree
50.91ms - 53.24msunsure 🔍
-0% - +6%
-0.04ms - +2.98ms
-unsure 🔍
-1% - +6%
-0.46ms - +2.84ms
previous-release
previous-release
49.71ms - 52.05msunsure 🔍
-2% - +4%
-1.23ms - +1.79ms
unsure 🔍
-5% - +1%
-2.84ms - +0.46ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
758.56ms - 771.41ms-unsure 🔍
-1% - +1%
-10.30ms - +8.95ms
unsure 🔍
-1% - +1%
-9.16ms - +8.28ms
tip-of-tree
tip-of-tree
758.49ms - 772.83msunsure 🔍
-1% - +1%
-8.95ms - +10.30ms
-unsure 🔍
-1% - +1%
-9.05ms - +9.52ms
previous-release
previous-release
759.52ms - 771.32msunsure 🔍
-1% - +1%
-8.28ms - +9.16ms
unsure 🔍
-1% - +1%
-9.52ms - +9.05ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
750.91ms - 761.69ms-unsure 🔍
-1% - +1%
-8.61ms - +6.62ms
unsure 🔍
-1% - +1%
-9.95ms - +6.34ms
tip-of-tree
tip-of-tree
751.91ms - 762.67msunsure 🔍
-1% - +1%
-6.62ms - +8.61ms
-unsure 🔍
-1% - +1%
-8.95ms - +7.33ms
previous-release
previous-release
751.99ms - 764.21msunsure 🔍
-1% - +1%
-6.34ms - +9.95ms
unsure 🔍
-1% - +1%
-7.33ms - +8.95ms
-

tachometer-reporter-action v2 for Benchmarks

@justinfagnani
Copy link
Collaborator

I worry about leaving the existing files in place, even with deprecation warnings. Will versions always be kept in sync? Even if so, will people get version mismatches between lit-html and ssr, like they already have?

I'd be templated to say that since these files are "experimental", that users just need to upgrade to the new locations. This would be the one outward breaking change in 3.0, and I think we can live with that.

@justinfagnani
Copy link
Collaborator

@augustjk do you need to set the base branch to 3.0 now?

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.

Nice work!

This PR should contain the updates to refer to the files in their new proper location though:

  • Update the package.json, .gitignore, rollup.config.js of lit, lit-html, and lit-element to remove entries for the support files.
  • Update lit-element/src/test/node-import.ts to use new files locations
  • Update SSR demos to use new files locations
  • Update 11ty plugin README and demo
  • Update labs/nextjs
  • Update testing README

import type {PropertyValues} from '@lit/reactive-element';
import {render, RenderOptions} from 'lit-html';
import {hydrate} from 'lit-html/experimental-hydrate.js';
import {HYDRATE_INTERNALS_ATTR_PREFIX} from '@lit-labs/ssr-dom-shim';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish I had caught this in the SSR-internals code review. I don't think the client packages should be depending on the shim package like this. I think we can define this attribute a bit like a protocol and just define it in multiple places so we don't need this dependency. At least I'd want to make a separate module for this in ssr-dom-shim.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makse sense. Filed #3769

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just copy the value into here now so that this package doesn't need to depend on the ssr-dom-shim?

packages/labs/ssr-client/package.json Outdated Show resolved Hide resolved
packages/labs/ssr-client/package.json Outdated Show resolved Hide resolved
packages/lit-html/src/experimental-hydrate.ts Show resolved Hide resolved
- Add more props to private-ssr-support
- Make TemplateInstance._parts a stable prop
- Add node build to rollup config
- Export everything from hydrate-lit-html
@augustjk
Copy link
Member Author

@justinfagnani Addressed the concerns.

Could you take a look at this commit in particular 8559720 ?

Trying to use the hydration modules from ssr-client package surfaced additional issues caused by it being outside so I had to make some changes.

Namely, I added more properties to lit-html/private-ssr-support.js and made TemplateInstance.prototype._parts into a stable part and renamed to _$parts.

@justinfagnani
Copy link
Collaborator

Namely, I added more properties to lit-html/private-ssr-support.js and made TemplateInstance.prototype._parts into a stable part and renamed to _$parts.

That probably had to be done anyway to support multiple hydrating versions of lit-html in a render tree.

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.

🎉

import type {PropertyValues} from '@lit/reactive-element';
import {render, RenderOptions} from 'lit-html';
import {hydrate} from 'lit-html/experimental-hydrate.js';
import {HYDRATE_INTERNALS_ATTR_PREFIX} from '@lit-labs/ssr-dom-shim';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just copy the value into here now so that this package doesn't need to depend on the ssr-dom-shim?

@justinfagnani justinfagnani mentioned this pull request Mar 30, 2023
10 tasks
@augustjk augustjk merged commit 575fb57 into main Apr 3, 2023
6 of 7 checks passed
@augustjk augustjk deleted the move-experimental-hydrate branch April 3, 2023 20:20
@lit-robot lit-robot mentioned this pull request Apr 3, 2023
@LasseRosenow
Copy link
Contributor

LasseRosenow commented Apr 3, 2023

After this got merged I have this error now:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'C:\Users\Lasse\Desktop\lit-ssr\node_modules\@lit-labs\ssr-client\node\index.js' imported from C:\Users\Lasse\Desktop\lit-ssr\node_modules\@lit-labs\ssr\lib\render-value.js

@augustjk
Copy link
Member Author

augustjk commented Apr 3, 2023

@LasseRosenow thanks for the report. this should be fixed in the latest release of @lit-labs/ssr-client.

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-client] Move experimental hydrate modules to @lit-labs/ssr-client
3 participants