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
Conversation
🦋 Changeset detectedLatest commit: b992c5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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
|
0922356
to
e7cfe49
Compare
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. |
@augustjk do you need to set the base branch to 3.0 now? |
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.
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 oflit
,lit-html
, andlit-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'; |
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 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.
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.
Makse sense. Filed #3769
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 you just copy the value into here now so that this package doesn't need to depend on the ssr-dom-shim?
- 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
@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 |
That probably had to be done anyway to support multiple hydrating versions of lit-html in a render tree. |
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.
🎉
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'; |
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 you just copy the value into here now so that this package doesn't need to depend on the ssr-dom-shim?
After this got merged I have this error now:
|
@LasseRosenow thanks for the report. this should be fixed in the latest release of |
Closes #3713
This PR can be reviewed commit by commit.
Copies contents of experimental files to new location
As
_$LH
is being pulled fromlit-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
andAttributePart
, type assertions were added.Update
package.json
files, exports, wireit configs, rollup to add new entry and node build.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 onlit-html
.experimental-hydrate-support.js
is side-effectful so added aconsole.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
andlit-element
(Lit3?) or just update the@lit-labs/ssr-client
?