Use auth headers in ImageBackground to get a FallbackAvatarURL.

[chrisbobbe]

As discussed yesterday, around here.

[gnprice]

Thanks for spotting and debugging this! Small comments below.

[gnprice]

The const UserAvatar = function UserAvatar… form seems a bit redundant -- I'd rather write the name just once. How about

function UserAvatar(props: Props) {

?

I'm choosing a non-arrow function so that I can give the function a
name (the same name as the variable where we store it). I think
React (understandably) has trouble putting a name to arrow functions
and anonymous non-arrow functions in stack traces and in the dev
tools, though I haven't recently tested this.

So I believe we actually have a Babel plugin operating that turns things into this form, given the form with arrow functions we usually use. Namely, @babel/plugin-transform-function-name, which is pulled in by metro-react-native-babel-preset (see node_modules/metro-react-native-babel-preset/src/configs/main.js.)

That arrangement actually seems slightly silly to me -- why not just write something like

function UserAvatar(props: Props) { … }

in the first place? 😛 It's shorter, even, than our usual (for functions) form

const UserAvatar = (props: Props) => { … }

But that arrangement is what we implicitly rely on in a lot of the app.

[chrisbobbe]

That arrangement actually seems slightly silly to me -- why not just write something like

function UserAvatar(props: Props) { … }

in the first place? 😛

Ah, right—I left this out of my explanation; oops. I have never favored this form because, like with var, it means I have to think about hoisting (doc).

But in this case it's probably fine; React component files aren't generally long (or shouldn't be, anyway). 🙂

[gnprice]

I see, that's helpful. It seems quite likely that's been other people's thinking too, and has helped motivate the const foo = () => { … } style.

I think the term "hoisting", which suggests an action of transforming the code into some other code, makes what's going on here more complicated than it needs to be. (You can see some ambivalence about it right at the top of that doc.)

With var there's some surprising semantics because you can have a declaration separate from an initialization, and "hoisting" may be a helpful idea for thinking about those semantics.

But with function, there's no such complication -- a function declaration function foo() { … } just means there's a function, and it's called foo, and you can call it from anywhere under that scope. If it's at module toplevel, you can call it from anywhere in the module.

There's really less to think about with this form than there is with const foo = function() { … } -- the latter form adds the complication that the function only exists in part of the code, namely after the declaration. So you have to put the code in a certain order, but only sometimes (only if you're referring to the function in code directly at top level, outside of any function... or in an IIFE... or more generally in code that can execute before the module's top-level code has completed execution), and you end up with lint rules like no-use-before-define that try to make things simple again by making the constraints tighter, but that make it impossible to write some kinds of code, or to write some other code naturally, and so we disable that rule in several files.

Whereas with a plain function declaration, you just have the function, and you can call it.

I agree that OTOH the complicated semantics of var, which "hoisting" is an attempt to explain, are a good reason to generally avoid using it.

[chrisbobbe]

There's really less to think about with this form than there is with const foo = function() { … }

What do you think of this small tweak to one of the examples in that doc?

catName("Chloe");

const suffix = "the cat"

function catName(name) {
  console.log("My cat's name is " + name + " " + suffix);
}

There, the order does matter—and it might be a bad surprise to someone working with the assumption that they could call catName from anywhere in the same scope. And our automated checks don't catch it; I just tried it out.

[gnprice]

Hmm, that is indeed more complicated 🙂 .

I think from the perspective above, the thing that's making that example complicated isn't the function -- it's that there's nontrivial logic happening at toplevel. Because there's that catName call happening at toplevel, it depends on suffix being present, and so it depends on the const suffix = … having already happened -- IOW there's a required order between those two statements. There wouldn't be such an ordering constraint if the catName call were inside a function definition (in any form), and were only invoked via an export.

In general it's a bad practice to have substantial logic happening at module toplevel, because it means that requiring a module can have side effects, or at a minimum can lead to spending time doing computation. I think it's rare in our code -- the only frequent patterns of top-level code that come to mind, beyond function expressions, are a handful of higher-order functions like connect that do no visible work up front.

[chrisbobbe]

it's that there's nontrivial logic happening at toplevel

True, it might be happening at toplevel—but it might not, right;

class IosCompliantAppleAuthButton extends PureComponent<Props, State> {
  state = {
    isNativeButtonAvailable: undefined,
  };

  async componentDidMount() {
    this.setState({ isNativeButtonAvailable: await AppleAuthentication.isAvailableAsync() });
  }

  render() {
    const { style, onPress, theme } = this.props;
    const { isNativeButtonAvailable } = this.state;
+   
+   catName("Chloe");
+
+   const suffix = "the cat"
+   
+   function catName(name) {
+     console.log("My cat's name is " + name + " " + suffix);
+   }
+   
    // [etc.]
  }
}

Though in a case like that, you'd really have to be trying to make the mistake, I guess.

[gnprice]

Yeah. And I'd look a lot more skeptically at a function declaration inside a function. I guess in fact one way to look at that is:

• the module toplevel is for definitions, and not logic -- so function statements are fine, and function expressions with long complex bodies (as in const f = (…): … => { … }) are the bread and butter of our codebase, but nontrivial logic to be executed immediately requires care;
• a function body is for logic -- so function statements, and for that matter complex function expressions, make things complex and require care, but nontrivial logic is at home;
• and one reason that's good is that mixing definitions and logic at the same level leads to surprises like this one.

[chrisbobbe]

Mmm, right; that's a really helpful way of thinking about it.

[gnprice]

This suggests we can probably benefit from a simple type-wrapper for useSelector, in our src/react-redux.js -- one that just specifies that the callback's argument is of type GlobalState.

I can try adding that after this is merged (or while merging it.)

[chrisbobbe]

Sure, sounds good—thanks!

[gnprice]

I'm puzzled now at why this hasn't applied all along to uploaded avatars. Discussion in chat.

[gnprice]

Thanks again! Merged, with a useSelector type-wrapper added on top.