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.

Sign up for GitHub

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

Jump to bottom

[labs/react] Add Node build #3410

Closed
wants to merge 4 commits into from
Closed

[labs/react] Add Node build #3410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e48176a
Add node build for server rendering
augustjk Oct 31, 2022
1577bb4
Add changeset
augustjk Oct 31, 2022
fe450aa
Don't shim HTMLElement
augustjk Oct 31, 2022
b67c03a
Check for presence of HTMLElement even in Node
augustjk Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/eight-elephants-sparkle.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@lit-labs/react': minor
---

Add Node build for server rendering. This will allow wrapped components to be added to SSR React frameworks like Next.js.
1 change: 1 addition & 0 deletions .eslintignore
View file
Open in desktop
Expand Up @@ -241,6 +241,7 @@ packages/labs/observers/performance_controller.*
packages/labs/observers/resize_controller.*
packages/labs/observers/intersection_controller.*
packages/labs/react/development/
packages/labs/react/node/
packages/labs/react/test/
packages/labs/react/node_modules/
packages/labs/react/index.*
Expand Down
1 change: 1 addition & 0 deletions .prettierignore
View file
Open in desktop
Expand Up @@ -227,6 +227,7 @@ packages/labs/observers/performance_controller.*
packages/labs/observers/resize_controller.*
packages/labs/observers/intersection_controller.*
packages/labs/react/development/
packages/labs/react/node/
packages/labs/react/test/
packages/labs/react/node_modules/
packages/labs/react/index.*
Expand Down
1 change: 1 addition & 0 deletions packages/labs/react/.gitignore
View file
Open in desktop
@@ -1,4 +1,5 @@
/development/
/node/
/test/
/node_modules/
/index.*
Expand Down
14 changes: 13 additions & 1 deletion packages/labs/react/package.json
View file
Open in desktop
Expand Up @@ -20,17 +20,20 @@
".": {
"types": "./development/index.d.ts",
"development": "./development/index.js",
"node": "./node/index.js",
"default": "./index.js"
},
"./use-controller.js": {
"types": "./development/use-controller.d.ts",
"development": "./development/use-controller.js",
"node": "./node/use-controller.js",
"default": "./use-controller.js"
}
},
"files": [
"/development/",
"!/development/test/",
"/node/",
"/index.{d.ts,d.ts.map,js,js.map}",
"/create-component.{d.ts,d.ts.map,js,js.map}",
"/use-controller.{d.ts,d.ts.map,js,js.map}"
Expand All @@ -43,6 +46,7 @@
"test": "wireit",
"test:dev": "wireit",
"test:prod": "wireit",
"test:node": "wireit",
"checksize": "wireit"
},
"wireit": {
Expand Down Expand Up @@ -111,7 +115,8 @@
"test": {
"dependencies": [
"test:dev",
"test:prod"
"test:prod",
"test:node"
]
},
"test:dev": {
Expand All @@ -133,6 +138,13 @@
],
"files": [],
"output": []
},
"test:node": {
"command": "node development/test/node-render.js",
"dependencies": [
"build:ts",
"build:rollup"
]
}
},
"author": "Google LLC",
Expand Down
1 change: 1 addition & 0 deletions packages/labs/react/rollup.config.js
View file
Open in desktop
Expand Up @@ -10,4 +10,5 @@ import {createRequire} from 'module';
export default litProdConfig({
packageName: createRequire(import.meta.url)('./package.json').name,
entryPoints: ['index', 'create-component', 'use-controller'],
includeNodeBuild: true,
});
8 changes: 7 additions & 1 deletion packages/labs/react/src/create-component.ts
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@
* SPDX-License-Identifier: BSD-3-Clause
*/

const NODE_MODE = false;
taylor-vann marked this conversation as resolved.
Show resolved Hide resolved

// Match a prop name to a typed event callback by
// adding an Event type as an expected property on a string.
export type EventName<T extends Event = Event> = string & {
Expand Down Expand Up @@ -315,7 +317,11 @@ export function createComponent<
if (
eventProps.has(k) ||
(!reservedReactProperties.has(k) &&
!(k in HTMLElement.prototype) &&
// NODE_MODE check below is to support React SSR where HTMLElement
// might not be defined
!(NODE_MODE && typeof HTMLElement === 'undefined'
taylor-vann marked this conversation as resolved.
Show resolved Hide resolved
? false
: k in HTMLElement.prototype) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@augustjk augustjk Nov 1, 2022 •
edited

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding #3128, see #3410 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

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 

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@e111077 e111077 Nov 10, 2022 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

k in element.prototype)
) {
this._elementProps[k] = v;
Expand Down
25 changes: 25 additions & 0 deletions packages/labs/react/src/test/node-render.ts
View file
Open in desktop
@@ -0,0 +1,25 @@
/**
* @license
* Copyright 2022 Google LLC
* SPDX-License-Identifier: BSD-3-Clause
*/

// This file will be loaded by Node from the node:test script to verify that
// wrapped components can be server rendered by React.

import {createComponent} from '@lit-labs/react';
import {ReactiveElement} from '@lit/reactive-element';
import {customElement} from '@lit/reactive-element/decorators/custom-element.js';
import React from 'react';
import {renderToString} from 'react-dom/server.js';

@customElement('my-element')
class MyElement extends ReactiveElement {}

const Component = createComponent({
react: React,
tagName: 'my-element',
elementClass: MyElement,
});

renderToString(React.createElement(Component));