Add a ReactElementFactory typedef

[maxwellpeterson-wf]

When developing a large frontend application that consists of many different Dart package, all using React for the UI, it can be useful to pass around functions that return ReactElement. It is natural to want to define a typedef for such a function.

However, because of how typedefs work in Dart, all Dart packages that are using this typedef need to depend on some common dependency.

What better common dependency than react itself?!

[aviary3-wk]

Security Insights

No security relevant content was detected by automated scans.

Action Items

• Review PR for security impact; comment "security review required" if needed or unsure

• Verify aviary.yaml coverage of security relevant code

  Questions or Comments? Reach out on Slack: #support-infosec.

[kealjones-wk]

What better common dependency ~that~ than react itself?!

FTFY @maxwellpeterson-wf

[maxwellpeterson-wf]

re 1: I originally went to put this in over_react, but I thought that it'd make more sense to live in the package that defines ReactElement, and then re-export it from over_react in the same way.

2 is a good point... Seems like we'd need a major to roll it out if we wanted to be sure to avoid build failures

3 is an interesting point. We use the pattern of passing around "ui factories" extensively in the SDK, and I'm following that pattern for a similar use case. Tod and I cannot remember exactly why this became the pattern. One concern/question I have ...

In a WorkivaApp, the shell has a store that today stores ComponentFactorys (typedefed as dynamic Function() but in practice they're almost always ReactElement Function())

These factories are provided by consumers of the shell (think legacy experiences) and they get inserted into and removed from the React render tree a number of times depending on which experience should be rendered in the DOM at a given time. Each time an experience gets rendered, that ComponentFactory is invoked, resulting in a new instance of the ReactElement that the factory returned.

If we were to move away from passing factories, and instead passed ReactElement instances, we'd be storing that ReactElement instance in a store, and including it / excluding it from the rendering tree multiple times. Does that present any issues?

[greglittlefield-wf]

2 is a good point... Seems like we'd need a major to roll it out if we wanted to be sure to avoid build failures

Yeah 😕. Would you be okay just moving forward by redeclaring this typedef where necessary? I know having it in one place would be ideal, but having this typedef duplicated in the few places it's needed doesn't sound terrible to me.

If we were to move away from passing factories, and instead passed ReactElement instances, we'd be storing that ReactElement instance in a store, and including it / excluding it from the rendering tree multiple times. Does that present any issues?

I don't believe that presents any from a rendering behavior standpoint; React treats ReactElements as immutable instructions for rendering a component, and they can be reused in several different places in the tree simultaneously, and mounted/unmounted as needed.

Come to think of it though, creating a ReactElement does currently capture the currentOwner Fiber, which can have weird memory behavior if the ReactElement creation happens synchronously during the rendering another component, like we saw here: https://github.com/Workiva/web_skin_dart/pull/1672. Though, I wouldn't expect that problem to crop up in the shell, and from the looks of it it doesn't seem like a problem (when debugging addComponent , ReactCurrentOwner.current is null).

Using this pattern in the SDK or in other places where non-React code is interacting with React code is less of a concern for me (though my points about what drives component rerenders are still applicable); I mainly just don't want to encourage this pattern in places where we're completely inside React, or in consumer code.

[greglittlefield-wf]

Oh, one thing I forgot to mention is that React doesn't rerender a component if it's the same ReactElement in the same place in the tree, whereas if you have a new ReactElement each time, that component will get rerendered (unless it's memoized).

So, depending on when you're invoking the ReactElement factory function, you may get different rerendering behavior.

[maxwellpeterson-wf]

Would you be okay just moving forward by redeclaring this typedef where necessary? I know having it in one place would be ideal, but having this typedef duplicated in the few places it's needed doesn't sound terrible to me.

Unfortunately this doesn't work if those different declarations need to interact and type-check at all. In my case they do, so that was the impetus behind finding a home for a shared typedef declaration.

I will move forward with passing around ReactElement instance instead of a factory.

[greglittlefield-wf]

Unfortunately this doesn't work if those different declarations need to interact and type-check at all. In my case they do, so that was the impetus behind finding a home for a shared typedef declaration.

And from the PR description

However, because of how typedefs work in Dart, all Dart packages that are using this typedef need to depend on some common dependency.

I misread this the first time around and didn't get what you were saying; really sorry about that 😓.

@maxwellpeterson-wf Could you elaborate on where you're having issues? I was under the impression that the typedef didn't need to be shared, and should be interchangeable from a type-checking standpoint with equivalent typedefs or inlined function types.

For example:

typedef MyTypedef1 = int Function();
typedef MyTypedef2 = int Function();

void main() {
  int functionDecl() => 1;

  // All these types are interchangeable when it comes to assignments.
  MyTypedef1 functionVar1 = functionDecl;
  MyTypedef2 functionVar2 = functionVar1;
  int Function() functionVar3 = functionVar2;

  // All these types are interchangeable when it comes to declarations.
  print(functionDecl is MyTypedef1); // prints 'true'
  print(functionDecl is MyTypedef2); // prints 'true'
  print(functionDecl is int Function()); // prints 'true'
}

[greglittlefield-wf]

Were you perhaps running into behavior where a dynamic-returning function wasn't a ReactElement Function()?

For example:

void main() {
  dynamic dynamicReturn() {}
  int intReturn() => 1;

  print(dynamicReturn is dynamic Function()); // true
  print(dynamicReturn is void Function()); // true
  print(dynamicReturn is int Function()); // false
  print(dynamicReturn is Object Function()); // false

  print(intReturn is dynamic Function()); // true
  print(intReturn is void Function()); // true
  print(intReturn is int Function()); // true
  print(intReturn is Object Function()); // true
}

[maxwellpeterson-wf]

Huh, weird! I wonder what the error was that sent me down this path. I'm betting it was that a single file imported two libraries that defined the same typedef with the same name, so the names conflicted, and I misinterpreted that. I will see if I can reproduce though!