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
[labs/ssr-react] Add package for integrating Lit SSR to React #3605
Conversation
🦋 Changeset detectedLatest commit: 8d97073 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
|
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.
Just some preliminary thoughts/question, only looked at the README/package.json so far!
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.
Just through the readme mostly.
Thanks for the initial feedback! Marking as draft to redo some parts of this before putting it up for review again. |
fd6b0ff
to
aab935d
Compare
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.
🎉
"node": "./node/enable-lit-ssr.js", | ||
"default": "./enable-lit-ssr.js" | ||
}, | ||
"./jsx-dev-runtime": { |
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.
Add .js extension
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.
We can't do that as the compilers that do the transform don't add the .js
extension on the import line. See https://www.typescriptlang.org/play?jsx=4#code/PQKhAIAECsGcA8CSBbADgewE4BcDK6BXTAYwFMoAbAS2wFoKBDAI1mFlk1s1IeO3BDAAUAB5kAT1qkKpZKQB2-VJnSoAjAF4A5MtVqt4ANalx24+K0A+IQAsqQgN4BtNQBoATK4DMAXQB0yAyoABRU4BqW4CKwqAzyRiYaDlQAvpbJKSJssfKWAJQposASUjJyipZAA
packages/labs/ssr-react/package.json
Outdated
"LitElement", | ||
"ssr", | ||
"lit-ssr", | ||
"react" |
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.
add "web components" and "next". If there's a limit, I don't know if we need lit-element
and variations.
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
/** | ||
* @fileoverview Side-effect import meant to be loaded in the **browser** before | ||
* any user code is loaded. Installs hydration support for `LitElement`. | ||
*/ |
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.
why do we need this file in addition to lit/experimental-hydrate-support.js
?
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.
With projects like Next.js that use the same source code for both server and client, I figured it's a convenience for users to just import this once at the top, and it'll handle the monkey patching on the server, and hydration import on the client.
import 'lit/experimental-hydrate-support.js'; | ||
|
||
// eslint-disable-next-line import/extensions | ||
export {jsxDEV} from 'react/jsx-dev-runtime'; |
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.
Why doesn't this have Fragment and jsxs? Should it be an export *
?
Is there a link to docs on the module exports contract for this module?
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.
Good catch for the Fragment
. Added. I think it's better to be explicit rather than export *
so we know it exports everything that's needed. It doesn't have a separate jsxs
because in dev mode the jsxDEV
serves both purpose and gets a isStaticChildren
boolean as a fourth argument instead instead.
There was no canonical doc that specifies this contract. Things I referenced were:
- React's RFC: https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md
- Babel's release note: https://babeljs.io/blog/2020/03/16/7.9.0#a-new-jsx-transform-11154httpsgithubcombabelbabelpull11154
- React's own export: https://github.com/facebook/react/blob/9d111ffdfbcfee4b348a3d49c16f02cb718c896f/packages/react/jsx-dev-runtime.js
- Emotion's implementation: https://github.com/emotion-js/emotion/blob/acb72a45592881d9d1f72003b6db2e488b981599/packages/react/src/jsx-dev-runtime.js
- theme-ui's implementation: https://github.com/system-ui/theme-ui/blob/2d8bfc3ab29e54e925095ba0b5dbdb07bf7209db/packages/core/src/jsx-dev-runtime.ts
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 ask about export *
because if this module needs to implement a certain contract that we know that react/jsx-dev-runtime
does, then the easiest way to guarantee that we meet the contract is to export *
that module. It fits the spirit of monkey-patching that module, which is kind of what we're emulating.
|
||
import 'lit/experimental-hydrate-support.js'; | ||
// eslint-disable-next-line import/extensions | ||
export {Fragment, jsx, jsxs} from 'react/jsx-runtime'; |
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.
Should this be export *
?
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 like the explicit better and the exports here really shouldn't change.
* the server. It is simply a passthrough of `React.createElement` in the | ||
* browser. | ||
*/ | ||
export const createElement = wrapCreateElement(React.createElement); |
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 we not do this with an if (isServer)
check and have one module?
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.
We could but everything else in this package already uses the conditional export. What's the reason for wanting to consolidate to a single module using isServer
?
|
||
const {mode = 'open', delegatesFocus} = renderer.shadowRootOptions; | ||
const templateAttributes = { | ||
shadowroot: mode, |
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.
Change to shadowrootmode: mode
for compatibility with Safari and streaming SSR. We need to update our polyfill too.
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.
Should we keep the existing shadowroot: mode
as well for compatibility with older Chrome versions? Per Elliott's testing, it seems fine to have both present and will have the streaming behavior for supported browsers. The streaming DSD feature that also renames the attribute is estimated to ship in Chrome 111 https://chromestatus.com/feature/5161240576393216
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 we can launch with just the new attribute considering that we have no existing users. We'd have to deprecate the old attribute right away anyway.
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'm hesitant on launching with an attribute that doesn't work on any currently shipping stable browsers. Our core labs/ssr package also does both which means nested custom elements will get both attributes. We should match that behavior and deprecate the old attr at the same time as when we do so on in labs/ssr.
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.
"launch" is a loose term here. Does this PR put the package in releasable state? Chrome 111 goes beta next well and stable in 4 weeks. Safari TP will be promoted relatively soon.
Either way is ultimately fine, though I do want to remove the old attribute as soon as possib.e.
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 PR does put the package in a releasable state. I think removing the old attribute in sync with core labs/ssr makes sense.
|
||
// elementAttributes will be provided to React as props for the host element | ||
// for properly rendering reflected attributes | ||
const elementAttributes = |
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.
We should update the ElementRenderer interface to have a getAttributes()
API.
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.
Filed #3630
|
||
### Using the Automatic Runtime JSX Transform | ||
|
||
If your project is using the [runtime JSX transform](https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html), this package can serve as the JSX import source. |
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.
Do we have a solution at all for projects that use dependencies compiled with the runtime JSX transform? Do we need to note this 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.
It should be a very unlikely use case that affects external React components pre-compiled with auto runtime transform that directly make use of Lit components without the labs/react wrapper. But added a couple sentences to the bottom of this section regarding that use case.
* Renders the shadow contents of the provided custom element type with props. | ||
* Should only be called in server environments. | ||
*/ | ||
export const renderShadowContents = (type: string, props: {} | null) => { |
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.
There's almost nothing React-specific about this, right? This is generally useful because it wraps up the minimum steps needed to transform a tagName, attrs, and props from a call site into the reflected attrs and SR contents using ElementRenderer (something others have stumbled over as well). e.g. the Astro integration could use the same logic.
Can we move this to labs/ssr
, with an additional attrs
argument, and then this file would only add specific logic for separating React props into CE attrs & 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.
Naming nit: since this is more than just rendering "shadow contents" (technically it handles props -> reflected attributes as well), renderCustomElement
might be better? Also type
-> tagName
would be clearer.
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.
Renamed. I agree that this could very well be part of labs/ssr but want to punt it to make sure we can separately discuss the proper API for making it more general use.
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.
Filed #3656
}, | ||
}); | ||
|
||
return originalCreateElement( |
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.
Should be outside the if (shadowContents !== undefined)
test? We should still render a CE that doesn't render a SR for whatever reason.
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.
It would fall back to the return originalCreateElement(type, props, ...children)
outside the if (isCustomElement(type))
block in that case.
if (isCustomElement(type)) { | ||
const {shadowContents, elementAttributes, templateAttributes} = | ||
renderShadowContents(type, props); | ||
|
||
if (shadowContents) { | ||
const templateShadowRoot = createElement('template', { | ||
...templateAttributes, | ||
dangerouslySetInnerHTML: { | ||
__html: [...shadowContents].join(''), | ||
}, | ||
}); | ||
|
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 code is duplicated 4x, is there a way to factor it better? This can't all be just wrapCreateElement
I guess because there are slight differences in how children, keys, etc. are handled between the different fns, but maybe this inner part could be factored?
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.
The renderShadowContents()
(or maybe now-to-be-called renderCustomElement()
) was as much commonality I could pull out at the time. Though I think there might be a better way to deduplicate all of this. I'm thinking of actually creating a custom React element to handle the adding of template shadowroot so it would not need to have bespoke handling of the different transforms. I'll see if this idea pans out and push up a commit with 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.
@augustjk I would be tempted to TODO the deduplication and try a component in a follow-on PR.
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.
Yeah, it would just be implementation details and not affect the outward API so seems okay to punt that. @aomarks had the same thought while discussing offline.
Co-authored-by: Kevin Schaaf <kschaaf@google.com>
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.
Awesome!
Resolves #3588
This package contains tools for integrating Lit SSR with React for deeply server rendering Lit components.
A separate package for integrating these with Next.js is forthcoming(#3589)