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

✨ Initial amp-timeago Bento component #26507

Merged
merged 41 commits into from
Mar 5, 2020
Merged

✨ Initial amp-timeago Bento component #26507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f2c903d
amp-timeago v2
caroqliu Jan 27, 2020
4271a9c
Remove parseInt
caroqliu Jan 27, 2020
7c52654
Refactor logic into getFuzzyTimestampValue
caroqliu Jan 28, 2020
f979136
Lint
caroqliu Jan 28, 2020
0ec8325
Add IntersectionObserver to rerender
caroqliu Jan 28, 2020
f357dd4
Modify useIntersect
caroqliu Jan 28, 2020
1d4ed68
Refresh timestamp when entering view
caroqliu Jan 29, 2020
ec88e89
Move useIntersect
caroqliu Jan 29, 2020
170c55c
Pass props to getFuzzyTimestampValue
caroqliu Jan 29, 2020
178d13d
Use useRerenderer
caroqliu Jan 29, 2020
7546ac2
Add callback comments
caroqliu Jan 30, 2020
ef07436
Pass empty array to `useLayoutEffect`
caroqliu Jan 30, 2020
3ef8b19
Add timeago.js to build check allowlist
caroqliu Jan 30, 2020
08d869d
Remove useRerenderer and implement useInView
caroqliu Jan 30, 2020
b02dd4b
Update bind example on test page
caroqliu Jan 30, 2020
7d9c279
Correct amp-bind example text
caroqliu Jan 31, 2020
c4890a1
Refactor useIntersect as useInViewEffect
caroqliu Feb 11, 2020
45d6581
Rename use-intersect.js
caroqliu Feb 11, 2020
b8c30c2
Allowlist 3p timeago dependency
caroqliu Feb 11, 2020
b7b61f6
Restructure shape of useInViewEffect
caroqliu Feb 11, 2020
a6beb2d
Pass ref.current to useInViewEffect
caroqliu Feb 11, 2020
5755f4e
Clean up typing
caroqliu Feb 13, 2020
748f2f9
useInView -> useIntersect, useOnEnterViewport
caroqliu Feb 13, 2020
630ceb1
Reset isIntersecting in cleanup function
caroqliu Feb 14, 2020
78df30a
Restore useInView implementation
caroqliu Feb 20, 2020
4c4464a
Refactor new logic
caroqliu Feb 20, 2020
7eb6bfd
Share IntersectionOb across timeago instances
caroqliu Feb 21, 2020
9f14bfc
Replace useFnInView with useIsIntersecting
caroqliu Feb 21, 2020
225d596
Update DOM on enter viewport only
caroqliu Feb 21, 2020
ef30173
Move file + lint
caroqliu Feb 21, 2020
ea197ca
Delete file
caroqliu Feb 21, 2020
e018573
Rename timestamp to timestampRef
caroqliu Feb 21, 2020
1453b39
Initialize timestamp with null
caroqliu Feb 25, 2020
69242f2
Add type annotation to timestampRef
caroqliu Feb 26, 2020
96fba1f
Map elements to setters
caroqliu Feb 26, 2020
b112d05
Place IntersectionObserver in component
caroqliu Feb 28, 2020
395e1ae
Delete useIsIntersecting
caroqliu Feb 28, 2020
6b78d30
unobserve -> disconnect
caroqliu Feb 29, 2020
d7cd8bf
Array destructure
caroqliu Feb 29, 2020
fb36fd5
Specify props more granularly
caroqliu Mar 4, 2020
f790b35
Types
caroqliu Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove useRerenderer and implement useInView
  • Loading branch information
@caroqliu
caroqliu committed Feb 29, 2020
commit 08d869db64d0986354be42797dfcd65a1991b5c3
19 changes: 9 additions & 10 deletions extensions/amp-timeago/0.2/timeago.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,23 @@
* limitations under the License.
*/

import {createElement, useRef} from '../../../src/preact';
import {createElement, useRef, useState} from '../../../src/preact';
import {timeago} from '../../../third_party/timeagojs/timeago';
import {useIntersect} from '../../../src/preact/use-intersect';
import {useRerenderer, useResourcesNotify} from '../../../src/preact/utils';
import {useInView} from '../../../src/preact/use-intersect';
import {useResourcesNotify} from '../../../src/preact/utils';

/**
* @param {!JsonObject} props
* @return {Preact.Renderable}
*/
export function Timeago(props) {
const timestamp = getFuzzyTimestampValue(props);
const {0: timestamp, 1: setTimestamp} = useState(
getFuzzyTimestampValue(props)
caroqliu marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the timestamp on each render. And per spec it should only do so when the element returns back to the viewport, right? Why not just do it in the if (isIntersecting) {} and store the value in the state instead of isIntersecting?

Copy link
Contributor Author

@caroqliu caroqliu Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was under the impression that this approach will cost us rendering a fresh timestamp on prop mutation from amp-bind. Specifically this is the case that's important to support:
image

i.e. the following does nothing on prop mutation.

export function Timeago(props) {
  const [timestamp, setTimestamp] = useState(null);
  const ref = useRef(null);

  useEffect(() => {
    const node = ref.current;
    const observer = new IntersectionObserver(entries => {
      const last = entries[entries.length - 1];
      if (last.isIntersecting) {
        setTimestamp(getFuzzyTimestampValue(props));
      }
    });
    if (node) {
      observer.observe(node);
    }
    return () => observer.disconnect();
  }, []);

  useResourcesNotify();
  return createElement('time', {datetime: props['datetime'], ref}, timestamp);
}

We could update the timestamp on every render to handle prop mutation, but that both doesn't differentiate between the attempt to render on exit viewport and can also be consolidated into the approach we have now.

Copy link
Member

@samouri samouri Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So,

  • you want to update ts on either prop change or onEnterViewport
  • you do not want to update ts on random parent rerender or onExitViewport.

I think you can follow Dima's suggestion, but then also have a clause like this to update on prop change:

useEffect(() => setTimestamp(...), [props.timestamp])

Copy link
Contributor

Choose a reason for hiding this comment

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

but then also have a clause like this to update on prop change:

See reactjs/rfcs#107 (comment), and reactjs/rfcs#107 (comment). We should not do that.

We have two cases for intersecting rerendering now:

  1. false -> true
  2. true -> false

If we want to make it so that we only rerender when false -> true, we need to introduce a high order state helper. I had proposed something like:

function useRerenderWhenGoingTrue(initial) {
  const ref = useRef(initial);
  const [, setCount] = useState(0);

  const setter = useCallback(newValue => {
    const oldValue = ref.current;
    if (newValue === true && oldValue === false) {
      setCount((count) => count + 1);
    }
    ref.current = newValue;
  })

  return [ref.current, setter];
}

This will cause a rerender only when inIntersecting goes from false -> true:

const [isIntersecting, setIsIntersecting] = useRerenderWhenGoingTrue(false);

const inob = new IntersectionObserver(rs => {
  setIsIntersecting(rs[0].isIntersecting);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

But, I think the code was fine as is, it doesn't matter too much if timeago renders when exiting viewport.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rerunning effect on prop change is very common and no problem with it that I can think of. The "this pattern is shady" in the links, I believe, is with passing arrays around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the initial problem wasn't with the "renders when exiting viewport", but with the content changing on every external rerender with state/props being invariant.

);
const ref = useRef(null);
const entries = useIntersect(ref);
const last =
entries.length > 0 ? entries[entries.length - 1] : {isIntersecting: false};
const {isIntersecting} = last;
if (isIntersecting) {
useRerenderer();
const inView = useInView(ref);
if (inView) {
setTimestamp(getFuzzyTimestampValue(props));
}
useResourcesNotify();
return createElement('time', {datetime: props['datetime'], ref}, timestamp);
Expand Down
11 changes: 11 additions & 0 deletions src/preact/use-intersect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,14 @@ export function useIntersect(ref) {
}, [ref.current]);
return entries;
}

/**
* @param {object} ref
* @return {?Array<IntersectionObserverEntry>}
caroqliu marked this conversation as resolved.
Show resolved Hide resolved
*/
export function useInView(ref) {
const entries = useIntersect(ref);
const last =
entries.length > 0 ? entries[entries.length - 1] : {isIntersecting: false};
return last.isIntersecting;
}
30 changes: 2 additions & 28 deletions src/preact/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,7 @@
*/

import {getAmpContext} from './context';
import {
useContext,
useEffect,
useLayoutEffect,
useRef,
useState,
} from './index';
import {useContext, useEffect, useLayoutEffect} from './index';

/**
* @param {function()} callback
Expand Down Expand Up @@ -51,25 +45,5 @@ export function useResourcesNotify() {
if (notify) {
notify();
}
}, []);
}

/**
* @param {number} current
* @return {number}
*/
function increment(current) {
return current + 1;
}

/**
* @return {function()}
*/
export function useRerenderer() {
const state = useState(0);
// We only care about the setter, which is the second item of the tuple.
const set = state[1];
// useRef ensures the callback's instance identity is consistent.
const ref = useRef(() => set(increment));
return ref.current;
});
}