[labs/react] Add Node build #3410

augustjk · 2022-10-31T23:21:20Z

Resolves #3397

This adds a Node build for the @lit-labs/react package. ~~The Node build version will provide a minimal shim of an empty HTMLElement class to the createComponent module, using the same approach as reactive-element in #3156.~~

The Node build version skips the in HTMLElement.prototype if HTMLElement is not defined in global. In that case, relying on in element.prototype would be enough, since ReactiveElement would be extending an empty HTMLElement class from our Node build shim.

This approach will conflict with #3128, but it should just be a matter of moving over the NODE_MODE check to where HTMLElement reference got moved to. We can coordinate depending on which one gets merged first. cc: @taylor-vann

changeset-bot · 2022-10-31T23:21:23Z

🦋 Changeset detected

Latest commit: b67c03a

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

This PR includes changesets to release 1 package

Name	Type
@lit-labs/react	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 · 2022-10-31T23:21:40Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -3% - +2% (-0.72ms - +0.41ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 78.17ms - 82.38ms
lit-html-kitchen-sink: unsure 🔍 -3% - +3% (-0.74ms - +0.88ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -1% - +3% (-0.08ms - +0.26ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: slower ❌ 0% - 4% (0.25ms - 2.03ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +2% (-1.00ms - +0.86ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 665.33ms - 674.58ms
lit-html-kitchen-sink: unsure 🔍 -3% - +4% (-2.11ms - +3.11ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -0% - +3% (-0.05ms - +7.23ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.37ms - +1.27ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-2.05ms - +7.15ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 671.11ms - 678.85ms
reactive-element-list: unsure 🔍 -1% - +0% (-7.99ms - +1.79ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 106.0.5249.119
Sample size: 50
Built by: Benchmarks #4002
Commit: 9af59a5

render

Version	Avg time	vs
	78.17ms - 82.38ms	-

update

Version	Avg time	vs
	665.33ms - 674.58ms	-

update-reflect

Version	Avg time	vs
	671.11ms - 678.85ms	-

lit-html-kitchen-sink

Browser: chrome-headless 106.0.5249.119
Sample size: 30
Built by: Benchmarks #4002
Commit: 9af59a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	28.48ms - 29.69ms	-	unsure 🔍 -3% - +3% -0.74ms - +0.88ms	unsure 🔍 -2% - +4% -0.55ms - +1.03ms
tip-of-tree tip-of-tree	28.47ms - 29.55ms	unsure 🔍 -3% - +3% -0.88ms - +0.74ms	-	unsure 🔍 -2% - +3% -0.57ms - +0.91ms
previous-release previous-release	28.34ms - 29.35ms	unsure 🔍 -4% - +2% -1.03ms - +0.55ms	unsure 🔍 -3% - +2% -0.91ms - +0.57ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	78.53ms - 82.82ms	-	unsure 🔍 -3% - +4% -2.11ms - +3.11ms	unsure 🔍 -4% - +3% -3.05ms - +2.73ms
tip-of-tree tip-of-tree	78.69ms - 81.66ms	unsure 🔍 -4% - +3% -3.11ms - +2.11ms	-	unsure 🔍 -4% - +2% -3.10ms - +1.78ms
previous-release previous-release	78.90ms - 82.77ms	unsure 🔍 -3% - +4% -2.73ms - +3.05ms	unsure 🔍 -2% - +4% -1.78ms - +3.10ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	21.39ms - 21.65ms	-	unsure 🔍 -3% - +2% -0.72ms - +0.41ms	unsure 🔍 -1% - +1% -0.31ms - +0.22ms
tip-of-tree tip-of-tree	21.13ms - 22.23ms	unsure 🔍 -2% - +3% -0.41ms - +0.72ms	-	unsure 🔍 -2% - +3% -0.48ms - +0.71ms
previous-release previous-release	21.33ms - 21.80ms	unsure 🔍 -1% - +1% -0.22ms - +0.31ms	unsure 🔍 -3% - +2% -0.71ms - +0.48ms	-

lit-html-repeat

Browser: chrome-headless 106.0.5249.119
Sample size: 30
Built by: Benchmarks #4002
Commit: 9af59a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.05ms - 10.30ms	-	unsure 🔍 -1% - +3% -0.08ms - +0.26ms	unsure 🔍 -1% - +2% -0.09ms - +0.24ms
tip-of-tree tip-of-tree	9.97ms - 10.20ms	unsure 🔍 -3% - +1% -0.26ms - +0.08ms	-	unsure 🔍 -2% - +1% -0.17ms - +0.14ms
previous-release previous-release	9.99ms - 10.21ms	unsure 🔍 -2% - +1% -0.24ms - +0.09ms	unsure 🔍 -1% - +2% -0.14ms - +0.17ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	265.71ms - 271.54ms	-	unsure 🔍 -0% - +3% -0.05ms - +7.23ms	unsure 🔍 -1% - +2% -2.77ms - +5.76ms
tip-of-tree tip-of-tree	262.85ms - 267.22ms	unsure 🔍 -3% - +0% -7.23ms - +0.05ms	-	unsure 🔍 -2% - +1% -5.91ms - +1.71ms
previous-release previous-release	264.01ms - 270.25ms	unsure 🔍 -2% - +1% -5.76ms - +2.77ms	unsure 🔍 -1% - +2% -1.71ms - +5.91ms	-

lit-html-template-heavy

Browser: chrome-headless 106.0.5249.119
Sample size: 50
Built by: Benchmarks #4002
Commit: 9af59a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	52.18ms - 53.60ms	-	slower ❌ 0% - 4% 0.25ms - 2.03ms	unsure 🔍 -0% - +3% -0.22ms - +1.52ms
tip-of-tree tip-of-tree	51.22ms - 52.29ms	faster ✔ 0% - 4% 0.25ms - 2.03ms	-	unsure 🔍 -2% - +0% -1.22ms - +0.24ms
previous-release previous-release	51.75ms - 52.73ms	unsure 🔍 -3% - +0% -1.52ms - +0.22ms	unsure 🔍 -0% - +2% -0.24ms - +1.22ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	104.17ms - 105.34ms	-	unsure 🔍 -1% - +1% -1.37ms - +1.27ms	unsure 🔍 -1% - +1% -0.75ms - +0.88ms
tip-of-tree tip-of-tree	103.62ms - 105.99ms	unsure 🔍 -1% - +1% -1.27ms - +1.37ms	-	unsure 🔍 -1% - +1% -1.20ms - +1.43ms
previous-release previous-release	104.12ms - 105.26ms	unsure 🔍 -1% - +1% -0.88ms - +0.75ms	unsure 🔍 -1% - +1% -1.43ms - +1.20ms	-

reactive-element-list

Browser: chrome-headless 106.0.5249.119
Sample size: 50
Built by: Benchmarks #4002
Commit: 9af59a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	50.00ms - 51.36ms	-	unsure 🔍 -2% - +2% -1.00ms - +0.86ms	unsure 🔍 -3% - +1% -1.41ms - +0.68ms
tip-of-tree tip-of-tree	50.11ms - 51.39ms	unsure 🔍 -2% - +2% -0.86ms - +1.00ms	-	unsure 🔍 -3% - +1% -1.31ms - +0.72ms
previous-release previous-release	50.25ms - 51.84ms	unsure 🔍 -1% - +3% -0.68ms - +1.41ms	unsure 🔍 -1% - +3% -0.72ms - +1.31ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	679.08ms - 685.66ms	-	unsure 🔍 -0% - +1% -2.05ms - +7.15ms	unsure 🔍 -1% - +1% -4.82ms - +4.14ms
tip-of-tree tip-of-tree	676.61ms - 683.02ms	unsure 🔍 -1% - +0% -7.15ms - +2.05ms	-	unsure 🔍 -1% - +0% -7.31ms - +1.53ms
previous-release previous-release	679.66ms - 685.75ms	unsure 🔍 -1% - +1% -4.14ms - +4.82ms	unsure 🔍 -0% - +1% -1.53ms - +7.31ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	704.31ms - 710.80ms	-	unsure 🔍 -1% - +0% -7.99ms - +1.79ms	unsure 🔍 -1% - +0% -6.98ms - +2.82ms
tip-of-tree tip-of-tree	707.00ms - 714.31ms	unsure 🔍 -0% - +1% -1.79ms - +7.99ms	-	unsure 🔍 -1% - +1% -4.16ms - +6.20ms
previous-release previous-release	705.96ms - 713.30ms	unsure 🔍 -0% - +1% -2.82ms - +6.98ms	unsure 🔍 -1% - +1% -6.20ms - +4.16ms	-

_{tachometer-reporter-action v2 for Benchmarks}

packages/labs/react/src/create-component.ts

augustjk · 2022-10-31T23:58:09Z

I modified it a bit, just in case a user happens to bring in a DOM shim that does define HTMLElement. @aomarks could you take another look? Thanks!

packages/labs/react/src/create-component.ts

taylor-vann · 2022-11-01T00:16:30Z

This shouldn't be a whatever lands first situation.
React is a package that has around 300k downloads a week, we need to align on this

augustjk · 2022-11-01T00:30:34Z

This shouldn't be a whatever lands first situation. React is a package that has around 300k downloads a week, we need to align on this

I'm saying the conflict will be easy to resolve whichever gets merged first. This node build is strictly an addition that does not affect existing client side behavior in any way.

Here's how a merge of this into your PR with conflicts resolved would look like adde5b8#diff-70e2e47659157c4129a63acfad45aa00d53e5314c322b4acc9e677cb1cbaf97a

packages/labs/react/src/create-component.ts

taylor-vann · 2022-11-01T00:59:17Z

packages/labs/react/src/create-component.ts

+            // might not be defined
+            !(NODE_MODE && typeof HTMLElement === 'undefined'
+              ? false
+              : k in HTMLElement.prototype) &&


I feel like this ternary should be able to be done at the module level.

There should be a way to get the prototype or false outside of the ReactComponent.render function and outside of the createComponent function?

const SERVER_CHECK = <is node server> ? (Element || SomeShim) : HTMLElement;

then later we can use the appropriate prototype instead of both?

The ternary is potentially run for every property on every render. It should be only called when necessary.

Also the following in-out check disappears in #3128

!(k in HTMLElement.prototype) && ...

So this check might not conflict with the current code, but it does not fit in the proposed changes of #3128 which corrects the behavior these changes are based on.

The location of the code doesn't matter since terser will remove the side of the ternary that doesn't belong based on NODE_MODE for the "node" and "production" builds. I think it's cleanest if the conditional is close to where HTMLElement is referenced so we can remove it for the node build. ~~element and HTMLElement are not the interchangeable part here.~~ (this part was based on your first comment pre-edit.)

Regarding #3128, see #3410 (comment)

I understand the need for it to be close to the logic. But it's too much happening in an if statement. It makes it more difficult to read and maintain later on. Accounting for a fallback at the module level is more durable. This is technically a nested if statement except it's nested at the condition statement.

We are if-ing a prototype and a ternary inside a conditional statement does not read that way to me

element and HTMLElement are not the interchangeable part here.

HTMLElement is not used in #3128, this is what i mean by conflicts. Not conflicts as in git i mean conflicts as in implementation and correctness

Was lurking and not very plugged into this, but was reading the source on the branch and this line of code stood out to me.

it's too much happening in an if statement

Agree with Brian on this one. If you're keeping this logic, is there a way to rewrite this without hurting perf? Nested if I think would be okay here if only for readability

Yup. Now with the reference to HTMLElement moved to a lifecycle that won't be called on the server, this gnarly condition here won't be necessary. To address prop/attribute handling during server-render, I anticipate it'll be a much more straight forward single if (NODE_MODE) or if (isServer) check instead.

taylor-vann

see comments

taylor-vann · 2022-11-01T01:52:38Z

Also, I don't know if making createComponent aware of what MODE it is in the best approach.

Have we considered an HTMLElement shim of our own at the SSR side of things?

I'd like to leave createComponent as agnostic as possible since it can possibly be used on the server-side and client-side.

In the economy, like you pointed out, most developers would bring their own HTMLElement shim with them. And you've accounted for this use case in your changes.

The responsibility for the definition of HTMLElement in a particular environment exists outside of createComponent

I think the best approach would be to emulate that in our own server-side space. As in, @labs/ssr should provide a shim if one is not present for HTMLElement. It matches a developer journey without coupling createComponent to node

augustjk · 2022-11-01T23:40:50Z

@taylor-vann Appreciate the feedback, and apologies for not looping you in sooner!

Per offline discussion, we'll hold on this PR until #3128 goes in, which moves the HTMLElement reference out of render. I'll revisit afterwards to see if that's enough to not cause errors during initial server render by React, in which case the node build might not be immediately necessary.

For future SSR considerations, beyond making it not error, we'll have to look at the output by React's server rendering to consider which attributes should be added to the custom element SSR result, and whether we need to suppress React's own hydration warning for these wrapped custom elements as done in @kevinpschaaf 's early React SSR exploration work here https://github.com/lit/lit/blob/lit-react-ssr-old/packages/labs/ssr/src/lib/react/create-element.ts. I anticipate we'll eventually either need a node build or conditional code with isServer to handle that.

Putting this into draft for now.

augustjk · 2023-05-24T20:33:29Z

Superseded by #3885

augustjk added 2 commits October 31, 2022 15:57

Add node build for server rendering

e48176a

Add changeset

1577bb4

augustjk requested review from aomarks, justinfagnani and kevinpschaaf October 31, 2022 23:21

aomarks reviewed Oct 31, 2022

View reviewed changes

packages/labs/react/src/create-component.ts Outdated Show resolved Hide resolved

Don't shim HTMLElement

fe450aa

aomarks approved these changes Oct 31, 2022

View reviewed changes

Check for presence of HTMLElement even in Node

b67c03a

augustjk requested a review from aomarks October 31, 2022 23:58

aomarks approved these changes Nov 1, 2022

View reviewed changes

taylor-vann self-requested a review November 1, 2022 00:07

taylor-vann reviewed Nov 1, 2022

View reviewed changes

packages/labs/react/src/create-component.ts Show resolved Hide resolved

taylor-vann reviewed Nov 1, 2022

View reviewed changes

packages/labs/react/src/create-component.ts Show resolved Hide resolved

taylor-vann reviewed Nov 1, 2022

View reviewed changes

taylor-vann suggested changes Nov 1, 2022

View reviewed changes

augustjk marked this pull request as draft November 1, 2022 23:41

augustjk closed this May 24, 2023

augustjk deleted the react-node-build branch October 6, 2023 23:43

[labs/react] Add Node build #3410

[labs/react] Add Node build #3410

Conversation

augustjk commented Oct 31, 2022 • edited

changeset-bot bot commented Oct 31, 2022 • edited

🦋 Changeset detected

github-actions bot commented Oct 31, 2022 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

augustjk commented Oct 31, 2022

taylor-vann commented Nov 1, 2022

augustjk commented Nov 1, 2022

taylor-vann Nov 1, 2022 • edited

Choose a reason for hiding this comment

taylor-vann Nov 1, 2022 • edited

Choose a reason for hiding this comment

augustjk Nov 1, 2022 • edited

Choose a reason for hiding this comment

augustjk Nov 1, 2022

Choose a reason for hiding this comment

taylor-vann Nov 1, 2022

Choose a reason for hiding this comment

taylor-vann Nov 1, 2022

Choose a reason for hiding this comment

e111077 Nov 10, 2022 • edited

Choose a reason for hiding this comment

augustjk Nov 10, 2022

Choose a reason for hiding this comment

taylor-vann left a comment

Choose a reason for hiding this comment

taylor-vann commented Nov 1, 2022 • edited

augustjk commented Nov 1, 2022

augustjk commented May 24, 2023

augustjk commented Oct 31, 2022 •

edited

changeset-bot bot commented Oct 31, 2022 •

edited

github-actions bot commented Oct 31, 2022 •

edited

taylor-vann Nov 1, 2022 •

edited

taylor-vann Nov 1, 2022 •

edited

augustjk Nov 1, 2022 •

edited

e111077 Nov 10, 2022 •

edited

taylor-vann commented Nov 1, 2022 •

edited