🏗 Allow rendering plain DOM using JSX by alanorozco · Pull Request #36640 · ampproject/amphtml

alanorozco · 2021-10-27T22:01:15Z

Related to #36459

Provides a replacement for existing rendering methods that's more powerful and convenient:

	JSX	imperative DOM	`htmlFor`	`simple-template` (Stories)
like HTML	✅	🚫	✅	🚫
dynamic attrs	✅	✅	🚫	✅
dynamic children	✅	🚫	🚫	sort-of
event handlers	✅	🚫	🚫	🚫
nesting	✅	🚫	🚫	🚫

A minimal implementation of JSX is included as #core/dom/jsx.

This library adds ~0.22K of size to any given bundle. By replacing callsites of previous rendering methods, the overall size of a bundle is roughly neutral or smaller.

This change does not change any implementation code. A migration of existing templates and renderers will follow-up.

build-system/eslint-rules/preact.js

alanorozco · 2021-10-27T23:05:10Z

extensions/amp-story/1.0/amp-story-access.js

-        }
-
-        return drawerEl;
+        return <BlockingNotification logoSrc={logoSrc} />;


It's optional to enable this syntax by using Capitalized function names and taking a props object. Conversely, renderNotificationElement() is called as a normal function below.

Using <Component> syntax does come with some niceties, like the ability to nest components and receive children.

Still reviewing, but do we provide the ESLinting rules that would make these situations super clear and less prone to subtle errors?

e.g someone attempts to return <blockingNotification> instead of <BlockingNotification>.

Not a blocker, but good to enhance quickly if we dont already provide.

Tracking on #36679

amp-owners-bot · 2021-10-28T16:13:39Z

Hey @erwinmombay! These files were changed:

build-system/eslint-rules/preact.js

Hey @jridgewell! These files were changed:

build-system/eslint-rules/preact.js
src/core/dom/jsx.js

gmajoulet

Thank you for writing an example this is super helpful!

gmajoulet · 2021-10-28T16:17:22Z

extensions/amp-story/1.0/amp-story-access.js

+        <div class="i-amphtml-story-access-header">
+          <div
+            class="i-amphtml-story-access-logo"
+            style={logoSrc ? `background-image: url(${logoSrc});` : null}


This is awesome 🥇

+1000
This makes templating so easy/readable.

gmajoulet · 2021-10-28T16:28:16Z

Are we expecting any major diff in bundle size?

processprocess

This is awesome.
Approved with a couple of nits/questions.

test/unit/core/dom/test-jsx.js

processprocess · 2021-10-28T16:36:39Z

Are we expecting any major diff in bundle size?

A previous comment from @alanorozco :

Tradeoff in libraries is maintenance vs. size.

jsx-dom is imported from npm and weighs ~1.8k compressed

#utils/jsx-dom is ours (untested) and weighs ~0.3k compressed

We likely don't need most of what jsx-dom provides, particularly attribute tables if we ensure names are correct via linting (we already do this for Preact).

We will go with the jsx-dom util, so it will add ~0.3k compressed.
We will likely be able to refactor and delete code to make up for that. We can remove so many querySelectors now :)

alanorozco · 2021-10-28T17:01:29Z

@processprocess We will likely be able to refactor and delete code to make up for that.

More on this:

I'll remove changes to amp-story before submitting this PR, so there will be no actual impact. Since this is mainly meant for a new extension bundle, it's a clean slate.
If we migrate all templates in amp-story/ to JSX (replacing simple-template, htmlFor and createElementWithAttributes), the total compressed size decreases by ~0.2k. This is while leaving the content of the templates intact, not even refactoring to take advantage of JSX features! There's more room to improve if we refactor them.

In short, bundles that use this get a ~0.3k size penalty. But since it can replace htmlFor and createElementWithAttributes, and generates very compressible trees, size is neutral or smaller in a real world scenario.

samouri

This is super cool. Almost all LGTM, just had a few questions mostly FMI.
Definitely wait for a review from @jridgewell though, since I know he's also implemented his own version of jsx and is a SME here

src/core/dom/jsx.js

samouri · 2021-10-28T21:17:12Z

src/core/dom/jsx.js

+        return;
+      }
+      if (name.startsWith('on') && name.length > 2) {
+        element.addEventListener(name.toLowerCase().substr(2), value);


note: I double checked to confirm that all event names are lower case 👍

test/unit/core/dom/test-jsx.js

samouri · 2021-10-28T21:26:15Z

test/unit/core/dom/test-jsx.js

+  it('attaches event handlers from attributes', () => {
+    const onFooBar = env.sandbox.spy();
+    const element = <div onFooBar={onFooBar} />;
+    expect(element).to.not.have.attribute('onfoobar');


awesome. I wanted to ensure that <div onclick={onClick}/> wouldn't result in registering two event handlers, but since you don't keep the attribute in the final output, LGTM

src/core/dom/jsx.js

kristoferbaxter

Seems good to me, but I'd like to see @jridgewell's review too before proceeding.

kristoferbaxter · 2021-10-29T14:31:35Z

extensions/amp-story/1.0/amp-story-access.js

 * @return {!Element}
 */
-const getNotificationTemplate = (element) => {
-  return htmlFor(element)`


If we like this approach, a cleanup PR for the entire repo would be a great first task.

kristoferbaxter · 2021-10-29T14:33:36Z

extensions/amp-story/1.0/amp-story-access.js

-        }
-
-        return drawerEl;
+        return <BlockingNotification logoSrc={logoSrc} />;


Still reviewing, but do we provide the ESLinting rules that would make these situations super clear and less prone to subtle errors?

e.g someone attempts to return <blockingNotification> instead of <BlockingNotification>.

Not a blocker, but good to enhance quickly if we dont already provide.

kristoferbaxter · 2021-10-29T14:45:46Z

src/core/dom/jsx.js

+ * These features are omitted for the sake of bundle size:
+ *
+ * 🚫 Attribute names are not re-mapped.
+ *   - Use standard HTML attribute names on elements (`class`, not `className`).


If we plan to use this long term, a followup could include linting rules to prevent these features from being accidentally relied on.

Tracking: #36679

src/core/dom/jsx.js

jridgewell · 2021-10-29T21:12:45Z

extensions/amp-story/1.0/amp-story-access.js

@@ -1,3 +1,4 @@
+import * as Preact from '#core/dom/jsx';


Nit: I'm a bit worried we'll confuse this with actual Preact (and the ability to diff). Can we use a different name and @jsxPragma?

// input /** @jsx StaticTree.createElement */ <div/> // output StaticTree.createElement("div");

Babel

Using JsxStaticTree, with the rule enforcing Jsx if not Preact for the sake of simplicity. (Without the prefix we'd need to parse the pragma annotation, which would be fragile.)

I really wanted to do this but Closure is not playing nice.

WARNING - [JSC_BAD_JSDOC_ANNOTATION] Parse error. illegal use of unknown JSDoc tag "jsx"; ignoring it. Place another character before the @ to stop JSCompiler from parsing it as an annotation.

Tracking: #36680

alanorozco requested a review from processprocess October 27, 2021 22:01

alanorozco commented Oct 27, 2021

View reviewed changes

build-system/eslint-rules/preact.js Outdated Show resolved Hide resolved

alanorozco commented Oct 27, 2021

View reviewed changes

alanorozco force-pushed the jsx-in-stories branch 2 times, most recently from 9aa01dd to 8845b51 Compare October 28, 2021 16:13

alanorozco marked this pull request as ready for review October 28, 2021 16:13

amp-owners-bot bot requested a review from jridgewell October 28, 2021 16:13

alanorozco force-pushed the jsx-in-stories branch from 8845b51 to 385b183 Compare October 28, 2021 16:15

gmajoulet approved these changes Oct 28, 2021

View reviewed changes

processprocess approved these changes Oct 28, 2021

View reviewed changes

test/unit/core/dom/test-jsx.js Show resolved Hide resolved

test/unit/core/dom/test-jsx.js Outdated Show resolved Hide resolved

alanorozco requested a review from samouri October 28, 2021 17:13

alanorozco changed the title ~~🏗 Use JSX to render DOM in amp-story~~ 🏗 Render DOM using JSX in directories matching /amp-story* Oct 28, 2021

alanorozco force-pushed the jsx-in-stories branch from 10ce52f to 734787d Compare October 28, 2021 20:44

samouri reviewed Oct 28, 2021

View reviewed changes

samouri approved these changes Oct 28, 2021

View reviewed changes

samouri reviewed Oct 29, 2021

View reviewed changes

src/core/dom/jsx.js Outdated Show resolved Hide resolved

processprocess mentioned this pull request Oct 29, 2021

amp-story-shopping Template rendering I2I #36459

Closed

kristoferbaxter approved these changes Oct 29, 2021

View reviewed changes

jridgewell approved these changes Oct 29, 2021

View reviewed changes

alanorozco mentioned this pull request Oct 31, 2021

Use namespace other than Preact with #core/dom/jsx #36680

Open

alanorozco added 6 commits October 31, 2021 03:40

🏗 Use JSX to render DOM in amp-story

21c007b

Various details

8283fb3

spread always

177848b

test name

3c0f9af

simpler

2f73621

shorter

2dd5a96

alanorozco added 4 commits October 31, 2021 03:40

reuse condition

6105369

===

2e1541a

non-function test for on

eabac00

address review

a8b45bc

alanorozco force-pushed the jsx-in-stories branch from b8b69fd to a8b45bc Compare October 31, 2021 10:40

undo source changes

7259bbc

alanorozco changed the title ~~🏗 Render DOM using JSX in directories matching /amp-story*~~ 🏗 Allow rendering plain DOM using JSX Oct 31, 2021

type name

d6bc926

alanorozco merged commit 9d3407a into ampproject:main Oct 31, 2021

alanorozco deleted the jsx-in-stories branch October 31, 2021 23:03

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Nov 11, 2021

ampprojectbot added the PR use: In LTS label Dec 14, 2021

Conversation

alanorozco commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alanorozco Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amp-owners-bot bot commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmajoulet commented Oct 28, 2021

Uh oh!

processprocess left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

processprocess commented Oct 28, 2021

Uh oh!

alanorozco commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kristoferbaxter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alanorozco commented Oct 27, 2021 •

edited

Loading

alanorozco Oct 27, 2021 •

edited

Loading

amp-owners-bot bot commented Oct 28, 2021 •

edited

Loading

alanorozco commented Oct 28, 2021 •

edited

Loading

samouri left a comment •

edited

Loading