getDerivedStateFrom{Props,Catch} should update updateQueue.baseState

[acdlite]

Based on a bug found in UFI2.

There have been several bugs related to the update queue (and specifically baseState) recently, so I'm going to follow-up with some refactoring to clean it up. This is a quick fix so we can ship a patch release.

[gaearon]

This is hard to follow for me. What is your mental model for when it's safe and/or necessary to mutate baseState? How does this relate to processUpdateQueue that also mutates it?

[gaearon]

In particular I'm worried that a low-pri update can mutate baseState and then get interrupted. What guarantees we'll "disregard" that state-from-props next time we process a high pri update?

[bvaughn]

I think the fix makes sense. (Also agree with the suggestion to refactor.)

[acdlite]

We mutate baseState whenever we process the update queue. It represents the state before any of the updates in the queue are applied. It's different from memoizedState whenever we process an update but keep it in the queue. This happens when there are lower priority updates than what we're currently rendering, because we skip the low ones and come back to them later — i.e. it's what makes this work:

My mental model for getDerivedStateFromProps is a setState inside render, at the same priority as what we're currently rendering:

render() {
  if (this.props !== previousProps) {
    this.setState(getDerivedStateFromProps(this.props, this.state));
  }
  // ...
}

If we actually implemented it this way, then processUpdateQueue would have taken care of updating baseState and this bug probably wouldn't have happened. We decided not to use the updateQueue to implement getDerivedStateFromProps because we wanted to avoid adding updates to the current queue, which shares a persistent structure. I thought it would be more elegant. But this bug indicates that it's more complicated than we thought, and we should reconsider. This is one of the things I will address in my follow-up PR.

[bvaughn]

If we actually implemented it this way, then processUpdateQueue would have taken care of updating baseState and this bug probably wouldn't have happened.

But the nextState prop passed to componentWillUpdate would have been incorrect.

[acdlite]

@bvaughn What's a componentWillUpdate?

[maulberto3]

Hi @acdlite I just wanted to point out that my getDerivedStateFromProps() stopped working in my React 16.3.0., but as soon as I updated it to 16.3.1., it started working again.
Thanks.

However, my this.forceUpdate() -inside getDerivedStateFromProps()- stopped working. Maybe it's related? I am getting the TypeError: Cannot read property 'forceUpdate' of null error.

Edit: It's not working anywhere inside getDerivedStateFromProps(), but it is working inside custom methods of a React component. Does this help?

[gaearon]

this.forceUpdate (an instance method) is not supposed to work inside getDerivedStateFromProps (a static method). Why are you trying to use it? getDerivedStateFromProps() should be pure and have no side effects.

[maulberto3]

@gaearon Before, I just had some logic that used forceUpdate() inside componentWillReceiveProps(). Particularly, I used it in a Google Maps React component. As the map receives dropdown values, the map showed particular markers (http://crc-sen-maps.herokuapp.com/). Now the forceUpdate() part came when clicking on those markers, in order to show/hide InfoWindows for those particular markers.

Anyhow, since React is preparing ground for async, will refactor it as per your comment.

Thanks again.

P.S. I am using Michele Bertoli's React Google Maps React repo (https://github.com/MicheleBertoli/react-gmaps)

[gaearon]

forceUpdate() inside componentWillReceiveProps() doesn't seem very useful because the component is already updating. Hard to say more without seeing the code.

[maulberto3]

forceUpdate() inside componentWillReceiveProps() doesn't seem very useful because the component is already updating. Hard to say more without seeing the code.

I agree, very hard coded.
This is what I had inside componentWillReceiveProps() for showing/hiding InfoWindows for current on-screen markers, you can see the forceUpdate() thrown in there, for both onClick prop (markers) and onCloseClick prop (infowindows).

if (newProps.dropFiscValue !== this.props.dropFiscValue) {
      const obj1 = {}; //=> Markers, InfoWindows upon Dropdown Fiscalizacion Value
      obj1.markersFisc = {};
      obj1.iWsFisc = {};
      obj1.iWVisibleFisc = {};

      fisc_data.map((marker, index) => {
        // mapping over Área Inversiones data

        if (newProps.dropFiscValue === marker.empresa_responsable2) {
          const index2 = 'marker' + index.toString(); // id for Markers
          const markerFisc = {};

          const index3 = 'marker' + index.toString() + 'IW'; // id for infoWindows
          const iWFisc = {};

          const index4 = 'marker' + index.toString() + 'IW' + 'IsVisible'; // id for infoWindows State
          const iWState = {};
          iWState[index4] = false;

          iWFisc[index3] = (
            <InfoWindow
              key={marker.consec * -1}
              lat={marker.latitud}
              lng={marker.longitud}
              content={
                '<h2>' +
                marker.nombre_del_proyecto +
                '</h2> <hr/>' +
                '<b><b>' +
                marker.empresa_responsable +
                '</b></b><br/>' +
                '<b>Fase: </b>' +
                marker.fase +
                '<br />' +
                '<b>Dimensión (MW): </b>' +
                marker.dimesion +
                '<br />' +
                '<b>Inicio Planificación: </b>' +
                marker.fecha_inicio_planemiento +
                ' <br />' +
                '<b>Fin Factibilidad y Evaluación: </b>' +
                marker.fecha_fin_fact_eval +
                ' <br />' +
                '<b>Gasto Preinversión (millones de colones): </b>' +
                marker.gasto_preinv_millones_col +
                ' <br />' +
                '<b>Inspección ARESEP: </b><a target="_blank" rel="noopener noreferrer" href="https://www.aresep.go.cr">I-Cuatr 2018 (Ver Informe).</a> <br /> <br />' +
                '<u><b>Descripción</b></u>: ' +
                marker.justificacion +
                '<br /><br />'
              }
              onCloseClick={evt => {
                if (!iWState[index4]) {
                  iWState[index4] = true;
                  obj1.iWVisibleFisc[index4] = iWState[index4];
                  obj1.iWsFisc[index3] = iWFisc[index3];
                  this.forceUpdate();
                } else {
                  iWState[index4] = false;
                  obj1.iWVisibleFisc[index4] = iWState[index4];
                  obj1.iWsFisc[index3] = false;
                  this.forceUpdate();
                }
              }}
            />
          );

          markerFisc[index2] = (
            <Marker
              key={marker.consec}
              lat={marker.latitud}
              lng={marker.longitud}
              icon={'https://cdn.filestackcontent.com/MBJZhRRpeFe3YuCMbciA'}
              title={marker.nombre_del_proyecto}
              opacity={0.9}
              onClick={evt => {
                if (!iWState[index4]) {
                  iWState[index4] = true;
                  obj1.iWVisibleFisc[index4] = iWState[index4];
                  obj1.iWsFisc[index3] = iWFisc[index3];
                  this.forceUpdate();
                } else {
                  iWState[index4] = false;
                  obj1.iWVisibleFisc[index4] = iWState[index4];
                  obj1.iWsFisc[index3] = false;
                  this.forceUpdate();
                }
              }}
            />
          );
          Object.assign(obj1.markersFisc, markerFisc); // obj1.markersFisc[index2] = markerFisc[index2]; // put markers under its own root
          obj1.iWsFisc[index3] = iWState[index4]; // small trick, under IW put all false, then on clicks, update them accordingly
          obj1.iWVisibleFisc[index4] = iWState[index4]; // IW state
        }
      });

      this.setState(prevState => {
        return { obj1 };
      }); // state set: newly created obj
      console.log('----------');
    }

[gaearon]

Wow, this is really complex. Why isn't this logic in the render function?

[maulberto3]

Yes, it is complex, so sorry, my bad.
The reason for this complexity is because the React Google Map wrapper is coded in a way to accept React components as children and render them, i.e.:

<Gmaps lat={...} lng={...} ...props> {/* this is the Google Map mounted */}
  <Marker lat={...} lng={...} ...props> {/* this mounts a marker for a location */}
  <Marker lat={...} lng={...} ...props> {/* this mounts another marker for another location */}
  <Marker lat={...} lng={...} ...props> {/* and so on, and so forth */}_
</Gmaps>

According to the Dropdown values, the markers will be mounted/unmounted to the map (via react children components).

And, inside each rendered marker, I have set an onClick prop for showing/hiding those rendered markers' infowindows individually (hence, the forceUpdate()).

I guesss I should take this last logic out of the onClick props and render them in a different way, although still attached to each rendered markers, mmm...

[gaearon]

Are you familiar with React data flow in general?

Typically instead of manually managing a list of children like this, your render method should have something like

let markers = [];

// fill these markers based on state
// ...
// and then render them

return <Gmaps>{markers}</Gmaps>

[maulberto3]

Actually yes, I am using that in my render.

Particularly, upon deciding what markers to render (due to the selected Dropdowns), I create small containers, i.e.:
const objMarkersElecCont = Object.values(this.state.obj1.markersElec);

Then, I let React conditionally render (because there may be markers or not on-screen), i.e.:
{Array.isArray(objMarkersElecCont) ? objMarkersElecCont : false}

Note that this.state.obj1.markersElec which was created inside the (now unsafe) componentWillreceiveProps(), all inside the same Map component.

I wish you could briefly go to http://crc-sen-maps.herokuapp.com/, select a few markers and click on them, just to get a feel of all of this: The Map component is always mounted, I am just modifying in the fly its children.

[gaearon]

I'd suggest not putting React elements into state. I think this would make the code clearer.

[maulberto3]

That is certainly happening here, and will try to follow your advice.

[Sompongyaya]

ยายเข้า[email protected]