✨ Initial PreactBaseElement #25969

jridgewell · 2019-12-10T23:25:57Z

No description provided.

amp-owners-bot · 2019-12-10T23:26:03Z

Hey @rsimha, these files were changed:

build-system/compile/compile.js

Hey @erwinmombay, these files were changed:

build-system/eslint-rules/no-import.js

lgtm-com · 2019-12-10T23:46:36Z

This pull request introduces 1 alert when merging 9cbb946 into 67d1415 - view on LGTM.com

new alerts:

1 for Unreachable statement

build-system/babel-plugins/babel-plugin-transform-simple-object-destructure/index.js

examples/amp-date-display.amp.html

extensions/amp-date-display/0.2/amp-date-display.js

src/preact-base-element.js

src/preact/context.js

src/preact/utils.js

extensions/amp-date-display/0.2/test/validator-amp-date-display.html

dvoytenko · 2019-12-20T19:27:04Z

extensions/amp-date-display/0.2/amp-date-display.js

+  const data = /** @type {!JsonObject} */ (getDataForTemplate(props));
+  useResourcesNotify();
+
+  return render(data, props['children']);


So, what are the children in this case?

It's a <slot> JSX node in AMP/Bento mode, and whatever JSX nodes the dev passes in React mode.

dvoytenko · 2019-12-20T19:29:15Z

extensions/amp-date-display/0.2/amp-date-display.js

+  );
+  host.dispatchEvent(event);
+
+  return null;


To clarify, this does not causes Preact to clear out all DOM parent's children, right?

dvoytenko · 2019-12-20T19:30:14Z

extensions/amp-date-display/0.2/amp-date-display.js

+ * @return {*} TODO
+ */
+function AmpDateDisplayComponent(props) {
+  const render = props['render'];


Isn't JsonObject there to be able to just use dot property access, object pattern, etc? This might matter I think also because there sometimes d.ts files used to define the props and thus they could be open to minification.

JsonObject is the opposite, if forces computed foo['bar'] access. As a public interface, I think it's required that these never be minified.

But props are really meant to be compilable symbols. Even further, I've recently seen mainly the following form used and find it very helpful:

- function AmpDateDisplayComponent(props) { - const { render } = props; + function AmpDateDisplayComponent( { render } ) {

It really helps to get the shape of the component at a glance.

We don't necessarily need to "get there" on the first PR. But I think we need to have a general agreement on the direction.

dvoytenko · 2019-12-20T19:32:49Z

extensions/amp-date-display/0.2/amp-date-display.js

+ */
+function AsyncRender(props) {
+  const children = props['children'];
+  const {0: state, 1: set} = useState(children);


Why not just array form?

Array destructuring requires the iterator protocol (which also requires symbols). Using object destructuring will be smaller for legacy builds, and faster all the time.

dvoytenko · 2019-12-20T21:50:34Z

extensions/amp-date-display/0.2/amp-date-display.js

+ * @param {!JsonObject} props
+ * @return {*} TODO
+ */
+function AmpDateDisplayComponent(props) {


A nit, but I'd like the React-only stuff to go into a separate module.

dvoytenko · 2019-12-20T21:51:00Z

src/preact-base-element.js

+ * limitations under the License.
+ */
+
+import {Deferred} from './utils/promise';


WDYT about this one?

dvoytenko · 2019-12-20T21:52:02Z

src/preact-base-element.js

+    /** @override */
+    isLayoutSupported(layout) {
+      const layoutSupported = opts.isLayoutSupported;
+      return layoutSupported ? layoutSupported.call(this, layout) : true;


Hmm. It's ok to delay this to followup PRs, but let's touch base one why this shouldn't just be a simple class extends way. Is it only because of props?

I just haven't converted it yet. We can immediately change it in a follow up.

dvoytenko · 2019-12-20T21:55:16Z

src/preact/context.js

+export function getAmpContext() {
+  return (
+    context ||
+    (context = createContext({


Would it make sense for us to do an exception for side-effects in this case? I'm thinking this class is part of the AMP's React base that could be exportable as it's own library. So the closer we could get to the canonical state for React the better for these modules.

For use internally, this will be no different. If we export it externally, we could do a code mod? But either way, they'll be different context instances in different modules, and changing it doesn't help.

Yeah. So I'm wondering why not just allow side-effects in this case to make this more-or-less a classical Preact/React-style declaration?

One of my goals this year is to eliminate all side-effects, so that transitive dependencies don't accidentally cause a bunch of unintentional headaches. So, I'd rather we leave this as a function call.

dvoytenko · 2019-12-20T21:56:21Z

src/preact/utils.js

+/**
+ * @return {function()}
+ */
+export function useRerenderer() {


Can we remove this for now?

samouri · 2020-01-09T08:18:44Z

src/preact/base-element.js

+            {current: element.getRootNode().getElementById(value)}
+          : value;
+      props[name] = v;
+    }


Nit: What do you think about flattening the ternary into a series of else-ifs?

if (value == null) { props[name] = def.default; } else if (def.type == 'number') { props[name] = Number(value) } else if (def.type == 'Element') { // TBD: what's the best way for element referencing compat between // React and AMP? Currently modeled as a Ref. props[name] = {current: element.getRootNode().getElementById(value)} } else { props[name] = value; }

dvoytenko · 2020-01-10T17:43:41Z

src/preact/context.js

+export function getAmpContext() {
+  return (
+    context ||
+    (context = createContext({


Yeah. So I'm wondering why not just allow side-effects in this case to make this more-or-less a classical Preact/React-style declaration?

dvoytenko · 2020-01-10T17:46:11Z

src/preact/slot.js

+ * @param {string} attr
+ * @param {boolean} on
+ */
+function toggleAttribute(element, attr, on) {


This should already be in dom.js. I think I stole it from there.

dvoytenko · 2020-01-10T17:46:31Z

src/preact/slot.js

+ * @param {*} o2
+ * @return {boolean}
+ */
+function objectsEqualShallow(o1, o2) {


Let's move to object.js?

dvoytenko · 2020-01-10T17:48:49Z

extensions/amp-date-display/0.2/date-display.js

+export function DateDisplay(props) {
+  const render = props['render'];
+  const data = /** @type {!JsonObject} */ (getDataForTemplate(props));
+  useResourcesNotify();


I'm still struggling this this call. I feel like if we constraint it strictly to only be in the RenderDomTree that should be sufficient? Then we have a nice separation of concerns: DateDisplay functions more or less strictly within the React-style. Whole RenderDomTree adopts it to AMP.

Technically, yes.

But calling useResourcesNotify() will be a requirement for every Component, whether or not they actually rendering anything. It's because we'll be adding a lint rule for it, and there's no way for me to tell from the linter whether an DOM mutation could occur.

I think this is ok to start this way. But I'll still be asking and looking for something more composite here. We have pretty clear touchpoints where we deal with actual light/shadow DOMs between Slot and RenderDomTree. Also, per our React prototype, it's very likely that we will decide to wrap all/most of AMP elements into some form of a Root component to preserve the common layout semantics. The bottom line: let's roll with it for now, but we'll likely come back to it again.

dvoytenko · 2020-01-10T17:50:11Z

extensions/amp-date-display/0.2/render-dom-tree.js

+ * @param {!JsonObject} props
+ * @return {null}
+ */
+export function RenderDomTree(props) {


Should we move this to src/preact/ somewhere along with AsyncRender? These seem to be important classes to support a particular type of our components.

I was going to wait until we had other components to move it. Yes, it'll likely end up there.

dvoytenko · 2020-01-10T17:53:03Z

extensions/amp-date-display/0.2/amp-date-display.js

+ * @return {*} TODO
+ */
+function AmpDateDisplayComponent(props) {
+  const render = props['render'];


But props are really meant to be compilable symbols. Even further, I've recently seen mainly the following form used and find it very helpful:

- function AmpDateDisplayComponent(props) { - const { render } = props; + function AmpDateDisplayComponent( { render } ) {

It really helps to get the shape of the component at a glance.

We don't necessarily need to "get there" on the first PR. But I think we need to have a general agreement on the direction.

jridgewell · 2020-01-10T22:54:46Z

extensions/amp-date-display/0.2/date-display.js

+export function DateDisplay(props) {
+  const render = props['render'];
+  const data = /** @type {!JsonObject} */ (getDataForTemplate(props));
+  useResourcesNotify();


Technically, yes.

But calling useResourcesNotify() will be a requirement for every Component, whether or not they actually rendering anything. It's because we'll be adding a lint rule for it, and there's no way for me to tell from the linter whether an DOM mutation could occur.

jridgewell · 2020-01-10T22:55:18Z

extensions/amp-date-display/0.2/render-dom-tree.js

+ * @param {!JsonObject} props
+ * @return {null}
+ */
+export function RenderDomTree(props) {


I was going to wait until we had other components to move it. Yes, it'll likely end up there.

jridgewell · 2020-01-10T22:57:31Z

src/preact/context.js

+export function getAmpContext() {
+  return (
+    context ||
+    (context = createContext({


One of my goals this year is to eliminate all side-effects, so that transitive dependencies don't accidentally cause a bunch of unintentional headaches. So, I'd rather we leave this as a function call.

jridgewell · 2020-01-10T23:00:25Z

src/preact/slot.js

+ * @param {*} o2
+ * @return {boolean}
+ */
+function objectsEqualShallow(o1, o2) {


jridgewell · 2020-01-10T23:01:00Z

src/preact/slot.js

+ * @param {string} attr
+ * @param {boolean} on
+ */
+function toggleAttribute(element, attr, on) {


jridgewell · 2020-01-11T00:48:33Z

extensions/amp-date-display/0.2/date-display.js

+ * @return {Preact.Renderable}
+ */
+export function DateDisplay(props) {
+  const render = props['render'];


Continuing #25969 (comment):

But props are really meant to be compilable symbols.

We cannot change this. If we allow non-string access, then mangling will break the components between the amp-to-react boundary. Eg, we retrieve the display-in attribute, and set it to props['displayIn'] as part of collect props. But Closure will mangle the const { displayIn } = props that it receives in the Preact component (to QD, in my case). So we get undefined instead of the value we needed.

// Collect props does this: const props = { ['displayIn']: el.getAttribute('display-in')}; // Preact does this due to mangling: const displayIn = props.QD;

Even further, I've recently seen mainly the following form used and find it very helpful:

We can still do destructuring, if you'd like:

const { 'render': render } = props

It just has to use string keys and not the shorthand.

Yeah, I see your point. But it's so unidiomatic. What's our long-term approach here? Can we instruct closure to ignore minification for props? For these modules in their entirety? Should we transform these modules to {render} to {'render': render} before feeding them to Closure? I.e. let's take the step we need now, but what's our north star here?

I can see two solutions:

We transform any props access to string form before closure. This has the downside that any foo(props) call must also use props as the parameter name, which is extremely difficult to lint for. We may be able to get away with it if we force the type to still be a !JsonObject, but then our devs will sometimes have to write string keys for JsonObjects and sometimes not.

We extern the props definition for every component. I think this is actually the better solution. It just requires us to locate the extern file somewhere else, which is annoying.

IMHO, I agree. I think either solution (even maybe converting d.ts to externs) are good for what we want. React-style is a bit of a different domain language, especially if we allow jsx. So special transformers are not a crazy idea for certain.

dvoytenko · 2020-01-14T22:22:40Z

One of my goals this year is to eliminate all side-effects, so that transitive dependencies don't accidentally cause a bunch of unintentional headaches. So, I'd rather we leave this as a function call.

But any major concerns about moving too far away from the "canonical" React style for this? E.g. we could explicitly allow this use for contexts and nothing else.

jridgewell · 2020-01-15T00:08:24Z

But any major concerns about moving too far away from the "canonical" React style for this? E.g. we could explicitly allow this use for contexts and nothing else.

For this, I don't see useContext(getContext()) call as being materially different than useContext(Context). I don't think anyone will be confused by this.

This plugin transforms the collquial name into the fully qualified import path. This is necessary for the new preact integrations, since `preact/hooks` actually does `import * from 'preact'`. We can't directly import `preact` like that, it has to be the `preact/dist/preact.module.js` path so Closure will work correctly.

dvoytenko · 2020-01-16T00:33:26Z

For this, I don't see useContext(getContext()) call as being materially different than useContext(Context). I don't think anyone will be confused by this.

Certainly in this case it doesn't matter. We should even simply create a static hook useAmpContext() for this. But I'm bringing this up because:

Contexts are used inline potentially a lot for some type of components. E.g. List->ListItem, and similar. Sometimes such context objects could even be confined to a single module. If we don't allow side-effects for this, this if (!exists) {create()} factories will spread pretty quickly.
It's a pattern in React. Other side-effects include const Component = forwardRef(...) and high-order components, and maybe some other.
Otherwise, React and AMP are very much aligned: we both hate side effects. The forwardRef, createContext and such are simply not side-effects for React, but rather a declaration type. So if we can bridge this gap - we'll be more idiomatic, but both ecosystems would "get what they want".

jridgewell requested review from dvoytenko and dreamofabear December 10, 2019 23:25

googlebot added the cla: yes label Dec 10, 2019

dvoytenko suggested changes Dec 12, 2019

View reviewed changes

honeybadgerdontcare reviewed Dec 12, 2019

View reviewed changes

extensions/amp-date-display/0.2/test/validator-amp-date-display.html Outdated Show resolved Hide resolved

jridgewell mentioned this pull request Dec 12, 2019

🐛 Fix simple-object-destructure transform for check-types #26010

Merged

jridgewell force-pushed the react-base-element branch 3 times, most recently from 0c8d1ea to 8dc9452 Compare December 18, 2019 01:41

dvoytenko suggested changes Dec 20, 2019

View reviewed changes

jridgewell mentioned this pull request Jan 6, 2020

wg-runtime Status Update 2020-01-06 ampproject/wg-runtime#26

Closed

jridgewell force-pushed the react-base-element branch 2 times, most recently from f3257a5 to c9a65e9 Compare January 7, 2020 23:49

samouri reviewed Jan 9, 2020

View reviewed changes

dvoytenko reviewed Jan 10, 2020

View reviewed changes

jridgewell commented Jan 11, 2020

View reviewed changes

jridgewell mentioned this pull request Jan 13, 2020

✨ Proof of concept React BaseElement #24153

Closed

Gregable added the WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. label Jan 14, 2020

jridgewell added 8 commits January 14, 2020 19:08

Simplify ExportDeclaration visitor

4c77f96

Copyright year

3d3c910

Lint

664f7c2

Simplify

e86b29c

use package.json field to resolve to module

f73cb0c

Fix issues with module compilation in single-pass

e6c3171

Just import from the full worker-dom source

dfe9e31

jridgewell added 21 commits January 14, 2020 19:08

Fix tests

92a4c8f

Add preact modules to the sources list

fa3500d

Include preact package.json

a21c7ab

Fix name mangling of properties

d25bb39

Split out Preact components into their own files

c0720cc

Move PreactBaseElement into src/preact dir

3220352

Remove unused code

a939b65

Update copyright years

2052f69

Types

3d257f9

Remove unused imports

9ff0a7b

Patch preact with types

c399cb0

Fully type Preact API by using intermediate module

2280309

Strengthen types

24be8ef

Types and minification

b6a4bd1

React tests

f1f2656

Fix sinon imports

d10ec85

Rearrange methods

a6eaf78

Lint

9916714

Remove file

efdab25

Convert PreactBaseElement into a subclass-able subclass

e484395

Convert to static fields

5b60af5

dvoytenko approved these changes Jan 15, 2020

View reviewed changes

jridgewell force-pushed the react-base-element branch from 7e74e2e to 5b60af5 Compare January 15, 2020 00:10

jridgewell changed the title ~~WIP: ✨ Initial PreactBaseElement~~ ✨ Initial PreactBaseElement Jan 15, 2020

jridgewell merged commit 5ea027c into ampproject:master Jan 15, 2020

jridgewell deleted the react-base-element branch January 15, 2020 08:26

ampprojectbot added the PR use: In Beta / Experimental label Jan 24, 2020

jridgewell mentioned this pull request Jan 27, 2020

✨ Initial amp-timeago Bento component #26507

Merged

✨ Initial PreactBaseElement #25969

✨ Initial PreactBaseElement #25969

Conversation

jridgewell commented Dec 10, 2019

amp-owners-bot bot commented Dec 10, 2019 • edited Loading

lgtm-com bot commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jan 14, 2020

jridgewell commented Jan 15, 2020

dvoytenko commented Jan 16, 2020

amp-owners-bot bot commented Dec 10, 2019 •

edited

Loading

samouri Jan 9, 2020 •

edited

Loading

jridgewell Jan 11, 2020 •

edited

Loading