ampproject / amp-react-prototype Goto Github PK

See ampproject/amphtml#32195 for more details

The context: useResizeEffect hook. This hook takes two arguments: an element's ref and a callback function. The callback function is called whenever the element is resized, e.g. due to responsive styling. This hook only subscribes/unsubscribes to/from ResizeObserver when mounted/unmounted to reduce waste.

Currently the issue is that ref.current could change and this code does not take this into account. I see to options to remedy this:

/1/ Use dependency-based effect.

function useResizeEffect(ref, callback) {
  useEffect(() => {...}, [ref.current]);
}

The "bad" part here is that on initial rendering, the useEffect will be called twice: first with ref.current == null and then with ref.current == rendered-element. Thus this version does one extra subscribe/unsubscribe from the ResizeObserver.

/2/ Manually compare elements

This approach is more efficient, but really messy.

function useResizeEffect(ref, callback) {
  const prev = useRef();
  const observer = useRef();
  useEffect(() => {
    if (prev.current != ref.current) {
      if (observer.current) {
        observer.current.unobserve(prev.current);
      } else {
        observer.current = new ResizeObserver();
      }
      observer.current.observe(ref.current);
      prev.current = ref.current;
    }
    return () => {
      if (observer.current) {
        observer.current.disconnect();
        observer.current = null;
      }
    };
  }, [/*mount only*/]);
}

Which of these more idiomatic?

AMP's lightboxes are components with completely separate "roots" and lifecycle. They only supply imperative APIs such as:

lightbox.open().then(result => {
  // Lightbox has closed with a possible return value.
});

A classical approach in React would use a controlled style:

export function Demo() {
  const [open, setOpen] = React.useState(false);
  return (
    <div>
      <Button onClick={() => setOpen(true)}>Open</Button>
      <XDialog open={open} onClose={_ => setOpen(false)}>...</XDialog>
    </div>
  );
}

There are some benefits when using a controlled style. But there are also some nuances to consider:

1. Open state has to be controlled. This expands the required API surface. I.e. to use the XDialog, one must manage open state, supplied as a property, and handle onClose.
2. Out-of-lightbox closing has to be non-vetoable. This can probably be remedied by an additional onCloseRequest event callback.
3. AMP element itself has to be uncontrolled since DOM elements do not have controlled/uncontrolled concept. Thus we'd have to remap the controlled state to imperative DOM APIs.

Currently AMP elements are structured like this:

<amp-element>      <- AmpElement (custom element wrapper)
                     <- defines layout/size
  <container>      <- BaseElement (implementation)
                     <- adheres to the layout defined by AmpElement

Some of the most critical layout styles are:

• display: block|etc (but not "inline");
• proxies for width and height;
• position: relative|absolute|etc (but not "static");
• overflow: hidden.

React components follow AMP's BaseElement layer which means that they accept layout, but normally do not define their own static layout.

Besides layout, AmpElement is also responsible for showing placeholder and loading indicator. This is especially important since the placeholder and loading indicator are shown this way even before the BaseElement implementation script has been downloaded.

To still allow static layout in the React/AMP layer, we could provide AmpLayout component that could implement layout/placeholder/loading-indicator features via composition.

For some elements their layout size is important. For instance amp-fit-text picks the right font size given its content and available layout space. In AMP layout size is provided by the framework. However, in Bento, we do not want to depend on heavy framework features and instead need to enable the components themselves to react to size changes.

We can use a relatively new ResizeObserver to monitor size changes. See useResizeObserver as an example. There are some nuances however:

• It's near-impossible to polyfill well. ResizeObserver is available in Chrome and Firefox, and will be available in Safari very soon. That still leaves IE and Edge. Partial implementation is possible using window.onresize event, but it's a poor substitution.
• An instance of ResizeObserver is heavy. Sharing resize observers appears to improve performance up to 8x.

Very similar reasons should guide "in-viewport" approach with IntersectionObserver.

Most of composite elements would want to know their children, which can be easily monitored using mutation observer:

mutationObserver.observe(this.element, {childList: true});

This works well when the component does not differentiate the types of children. E.g. "all children are carousel's slides". When children have different designations we can use attributes to assign them different roles. For instance, a customized "next" button in the carousel can be marked up as <button arrow-next>. If these attributes can change after the initialization, we may need to observe each child for attribute changes as well. E.g.

mutationObserver.observe(child, {attributes: true, attributesFilter: ...});

The goal is to avoid this as much as possible given the additional performance and code management cost.

AmpContext allows control of rendering/loading via context properties. By default, loading can be enabled and applications can exercise a more fine-grained control using AmpContext.Provider. Additionally, we could standardize on a property for manual control as well to override AmpContext. E.g. loading-control=auto|manual|always or similar.

This is similar to property and contexts (see #48). We could place services as a map in the AmpContext or we could break them down a service-per-context. We could also consider passing services via component props.

In other words, it's either:

<AmpLightbox
    backButton={backButtonService}
    ></AmpLightbox>

vs

<BackButtonContext.Provider value={backButtonService}>
  <AmpLightbox></AmpLightbox>
</BackButtonContext.Provider>

Whether or not we use a single or per-service context: this could be a significant performance optimization since only the dependent components need to be re-rendered when a service changes.

Context: #29 (comment)

The goal is to find a clear way to separate DOM mutations from external script and components.

Usually we can separate user DOM updates from component updates using light and shadow trees. However, this is not always possible, such as the case of amp-selector component.

Let's consider the amp-selector component. For the amp-selector, all mutations are in the main DOM tree and all observable by the main script/css/etc. E.g. if the CE user wants to decorated a selected option within the amp-selector, the following CSS can be used:

.option[selected] {
  border: 1px solid red;
}

Currently this is virtually the only option for us to signify that a selection option (as a DOM element) is currently selected. This is definitely not great. It'd be much better from API point of view if we could set a custom state, e.g. .option:selected {...}. But custom states spec is still ways and ways away.

As a result, a subtree-based MutationObserver would see these mutations too. In other words the following are both valid code paths that mutate DOM:

1. In-component: user clicks on an option and the React component sets it as selected. Our Slot delegation updates DOM:

// A. React's onClick handler -> state
function AmpSelection.Option() {
  ...
  (<x onClick={() => setSelectedOptionState(props.option)} ...>)
  ...
}

// B. Slot's props.selected is set in React:
function AmpSelection.Option() {
  ...
  (<Slot selected={isSelectedOptionState(props.option)}>)
  ...
}

// C. Slot's side effect sets DOM attribute:

function Slot() {
  const domRef = useRef();
  useEffect(() => {
    const slot = domRef.current;
    ...
    // Update in the main DOM:
    const assignedOption = slot.assignedElements()[0];
    if (props.selected) {
      assignedOption.setAttribute('selected', '');
    } else {
      assignedOption.removeAttribute('selected');
    }
  });
  ...
}

2. Out-of-component mutation: a user script in the main document manually writes DOM:

button1.onclick = () => {
  option2.setAttribute('selected', '');
};

Update an attribute this way by the user script will trigger mutation observer and trigger React component re-rendering with the new value prop.

We need the mutation observer to synchronize DOM -> React component in the case /2/. But we don't really need mutation observer for /1/ since we ourselves ensure that DOM/React are in full sync. In general case, incorrectly working /1/ can cause cycles. So far the cycles in such mutations have been easy to work around or ignore. But in general case this is still a dangerous situation. It'd be nice to have a more "automatic" solution for this.

See #35 for context.

AmpLayout component would provide the following features:

• Define a child component's layout using AMP rules (layout, width, height, etc);
• Show placeholder until the child component is loaded;
• Show loading indicator until the child component is loaded.

To implement these features The AmpLayout component would:

1. Wrap the "child" component into another DOM element and use it to implement main UI rules.
2. Modify the "child" component's inline style to achieve the right layout. For instance, with the responsive layout the wrapper component, as well as placeholder, loading indicator, etc - should all be styled with position:absolute; inset: 0; width: 100%; height: 100%.

There could be several possible composition styles.

/1/ A static higher order component:

const AmpCarouselWithLayout = withLayout(AmpCarousel, {options});

Main drawback: all components have to have “WithLayout” pair.

/2/ JSX: Child component with cloning of children[0]:

<AmpWithLayout layout="responsive" width={300} height={200}>
  <AmpCarousel current-slide={1}>...</AmpCarousel>
</AmpWithLayout>

Implementation:

const child = props.children[0];
const decoratedChild = preact.cloneElement(
  child,
  {
    style: {
      // Preserve other styles:
      ...child.props.style,
      // Make the child component take all available space:
      position: 'absolute',
      inset: 0,
      width: '100%',
      height: '100%'
    }
  }
);

Question: any negative consequences of cloning?

/3/ JSX: wrapper with type property:

<AmpWithLayout layout="responsive" width={300} height={200}
    type={AmpCarousel}
    current-slide={1}>
  ...
</AmpWithLayout>

Implementation is very similar to the cloning case, but avoids the cloning:

const decoratedChild = preact.createElement(
  props.type,
  {
    ...props,
    style: {
      // Preserve other styles:
      ...props.style,
      // Make the child component take all available space:
      position: 'absolute',
      inset: 0,
      width: '100%',
      height: '100%'
    }
  }
);

Main drawback: we lose some type information by combining two components into one.

/4/ CSS approach:

We use a global stylesheet like this:

.amp-with-layout > * {
  position: absolute !important;
  inset: 0 !important;
  width: 100% !important;
  height: 100% !important; 
}

Main drawback: the global stylesheet is a global singleton side-effect and as such we'd have to correctly support server-side rendering, documents, shadow roots, embeds. We'd likely have to deal with FOUC.

Context:

Scroll event management is difficult for carousels, especially with smooth scrolling: the container can scroll slowly from slide to slide and at some point we need to update the state to the new "current" slide. Nuances:

1. The scroll events are generally aligned with rAF so if we update state on each scroll event, we will see a significant number of re-renderings.
2. Waiting for scrolling to end is complicated and often introduces some lag. It can also introduce some possibility of lost sync. This could be improved a lot with the new scrollend event.
3. We can throttle scroll events and that'd generally be acceptable performance-wise.
4. Smooth scrolling introduces another nuance: with single-pass rendering, onScroll recalculates the current slide and updates the component's state. In turn, the state change issues a side-effect to update the container's scrollLeft. This is a weird cyclical (and possibly buggy) behavior. All-in-all it might be better to just implement the scrollend.

Consider a carousel button customization. There are two approaches to place it in carousel:

1. Clone it as a vDOM component. This has a problem of losing styles, external event handlers, and an expected DOM hierarchy.
2. Slot it. The problem is that disabled attribute (Web spec issue) and event handlers (React issue) do not propagate from the slot to the distributed button.

As a result we created the implementation where we propagate state and events from the slot to button and back manually.

Use case: <amp-carousel current-slide={value}>. The currentSlide is currently a "state" value and this is preferable. We could consider making it a controlled property coupled with onSlideChange callback, but that'd expand the API surface for something very simple. However, what should happen if the currentSlide prop (or "current-slide" attribute in DOM) has been updated? The simplified useStateFromProp hook supports updating the internal state when a prop changes, but it's not without issues. For instance, assuming the default prop/attribute value of currentSlide = 0, if the currentSlide state has been updated and the script simply wants to reset it back to the first slide, setting the currentSlide back to 0 will not work since technically the property's value will not change.

When an area of DOM (or component subtree) becomes unrendered or uninteractive, we need to pause relevant components. Otherwise the playback will continue and use will have no way of finding where it’s coming from and how to stop it.

For playability we can use AmpContext context properties.

For imperative/declarative control, playback is very difficult to support declaratively. This we can use useImperativeHandle and export playback API including playState, play, pause, and other similar methods.

We can use AmpContext for all state properties such as renderable and playable. The main question is whether we breakdown the context per property or keep all the properties together. Breaking down per property means a lot of contexts, but also means fewer component updates.

Context: https://github.com/ampproject/amp-react-prototype/pull/8/files#diff-c554b9bd31408e663cff269f69a1c567R51

Issues:

1. Some component libraries take a stand that “back button” support has to be arranged by the caller outside the lightbox (controlled). But that causes numerous issues, including inability to make closing vetoable.
2. Back button support is HARD. Standard doesn’t exist.

Supporting back button involves the following considerations:

• Rollback of history pop for vetoable close is especially hard and requires in-depth knowledge of the stack to re-push the popped states. Some routers (e.g. ReactRouter) allow history-stack blocking for this. But it’s rare and usually buggy.
• A naive history.pushState can break a React router and can never be fully featured.

See also #43 on the discussion about controlled-vs-uncontrolled lightbox API.

The pattern:

const ref = useRef();
useEffect(() => {
  ref.current.addEventListener(...);
  return () => ref.current.removeEventListener(...);
}, [...])

return (
  <div>
     {props.mode === 1 ? <Mode1 ref={ref}/> : <Mode2 ref={ref}/>}
  </div>
);

In this pattern, it's hard to react safely to changes of ref.current, e.g. when a node mapped to it is deleted/changes. The "right" way to do this is to ensure that the deps array contains the same condition that affects ref. E.g. [props.mode] in the example above. However, it's not always obvious and easy to miss.

Some solutions are below.

/1/ Ask nicely for deps to be correct and hope for the best

Hopefully an "exhaustive deps" linter would not remove the extra dep.

/2/ Ban changing of Ref mapping.

I.e. disallow the example above. This could be hard with forwardRef.

/3/ Use state function instead of ref:

const [node, setNode] = useState();
useEffect(() => {...}, [node])
return <...><Mode1 ref={setNode}></...>

The negative: it forces the second rerender each time the ref changes.

/4/ Use a funky xEffectWithRef version.

It'd manage the ref value internally and could look something like this:

function useEffectWithRef(ref, effect, deps) {
  const unsubscribe = useRef(null);
  const prev = useRef(null);
  useEffect(() => {
    return () => doUnsubscribe(prev, unsubscribe);
  }, deps || []);
  useEffect(() => {
    const {current} = ref;
    if (current !== prev.current) {
      doUnsubscribe(prev, unsubscribe);
      prev.current = current;
      if (current) {
        unsubscribe.current = effect(current);
      }
    }
  });
}

The positive: it doesn't cause rerender.
A negative: one effect is executed each time, but it will almost always do nothing.

See #40

Most of AMP elements have standard "public" state, including:

• isBuilt
• isLoaded
• loadedPromise
• etc

And also AMP elements have some imperative APIs such as:

• executeAction
• unlayout
• pause/resume

The two questions are:

1. How to export these APIs to AMP framework.
2. Whether and how to export these APIs to React layer.

For imperative API exporting we can use forwardRef and useImperativeHandle. We need to be careful with such APIs however - changing their shape has to follow backward-compatibility rules.