Migrate Tribes page to React (finishing the feature)

[mrkvon]

Proposed Changes

• Finalize the migration of /tribes page to React
• api calls are made from React
• there is a remaining layer of angularjs for communicating between angular and react

Testing Instructions

• git checkout or switch to tribes-react-page, npm install, npm start
• go to /tribes
• See that the page works as expected; that the functionality has not degraded.
  • specifically, test when user is both signed in and signed out.
• FYI we have not migrated some google analytics calls. So they were removed.

Finishing work on #1133
Depends on #1137, #1143 and #1144, and whatever these depend on. The dependencies should be merged to tribes-react branch first and then this branch should be rebased. (update: dependencies merged)

[nicksellen]

Great, very nice to have a whole page in react, left a few discussion points.

Also, maybe this is a good time to get in the practise of adding tests, even if they quite basic - that we can render the page, and click on some tribes... or see them listed or something.

Edit: I see #1169 now :)

[nicksellen]

Async stuff in useEffect looks a bit messy, but I know this is the way it has to be because of the cleanup function return thing, just want a little moan about it.

I wonder if the aesthetics of any of these variations are more pleasing?

useEffect(() => {
  const fetchData = async () => {
    const tribes = await api.read();
    setTribes(tribes);
  };
  fetchData();
}, []);

useEffect(() => {
  async function fetchData() {
    const tribes = await api.read();
    setTribes(tribes);
  }
  fetchData();
}, []);

async function fetchData() {
  const tribes = await api.read();
  setTribes(tribes);
}

useEffect(() => {
  fetchData();
}, []);

... or ... inspired by https://github.com/rauldeheer/use-async-effect ... a small implementation of useAsyncEffect:

function useAsyncEffect(fn, deps) {
  useEffect(() => {
    let cancelled = false;
    const isCancelled = () => cancelled;
    fn(isCancelled);
    return () => { cancelled = true; };
  }, deps);
}

which would enable use like this:

useAsyncEffect(async isCancelled => {
  const tribes = await api.read();
  if (isCancelled()) return;
  setTribes(tribes);
}, []);

Happy without anything like this though, just exploring thoughts/options...

[mrkvon]

If I need to choose, I like the 3rd option of defining fetchData outside, or the original one. But this is the kind of stuff that's impossible to argue about. 🙂

The useAsyncEffect looks magical, but has some additional benefits, doesn't it? Cancellation... does it stop the async function when leaving the component? Interesting!

[nicksellen]

No need to change anything at all here.

The useAsyncEffect isn't magical, just wrapping up the pattern you can do without it:

useEffect(() => {
  let cancelled = false;
  (async () => {
    const tribes = await api.read();
    if (cancelled) return;
    setTribes(tribes);
  })();
  return () => { cancelled = true; };
}, []);

It doesn't actually stop the async function as they don't have that feature (there was a cancellable promises proposal, but that got cancelled), axios requests can be cancelled so it's possible to connect that up too.

[mrkvon]

I see. The useAsyncEffect makes sure the component doesn't get updated after it's unmounted. If I observe correctly.

[mrkvon]

used the suggested version 3 (fetchData defined outside useEffect)
21d2106

The useAsyncEffect may have additional benefits. To be used here and elsewhere. In other PR.

[mrkvon]

@nicksellen @simison Nick's feedback is incorporated. If you like, make another round of reviews. Otherwise I'll squash and merge to tribes-react in 24+ hours. Then I'll wait for approval of the tribes-react #1148 and rebase + merge that to master. The tests can be merged in a follow-up I believe.

[simison]

I found this little hard to understand (a couple of arrow functions and ternary in one line, with lots of tribe or tribes sounding things going on.). It's ok to leave as is but would there be any way explaining this with comment? 😅

Not a blocker. :-)

[nicksellen]

It was me that suggested to change it in this way, perhaps the clearest would be to extract a mapReplace function (which has come up in lodash discussion before, e.g. lodash/lodash#1967). Something like mapReplace(array, condition, item), which would replace the item at condition with item.

[mrkvon]

There is also https://github.com/kolodny/immutability-helper, but I've just added a TODO for this. It's unclear for me what would be a proper place for such helpers. Feels like reinventing a wheel a bit.

[simison]

/tribes page tests well. 👍

Tested both as authenticated and as non-authenticated.

FYI we have not migrated some google analytics calls. So they were removed.

I think we still get URL changes which is what matters.

[nicksellen]

It was me that suggested to change it in this way, perhaps the clearest would be to extract a mapReplace function (which has come up in lodash discussion before, e.g. lodash/lodash#1967). Something like mapReplace(array, condition, item), which would replace the item at condition with item.