React-mapbox-gl: Wrong fitBounds update check

[jniebuhr]

In the code in https://github.com/alex3165/react-mapbox-gl/blob/master/src/map.tsx#L329 the Map checks for reference equality first. If both are equal, there is no use for checking the length and contents since you have two pointers to the same Array.
But if you calculate your bounds somewhere and just return a new Array it'll zoom to the bounds even if the contents didn't change. I would suggest removing the line.

[jniebuhr]

I think the following would be good:

      if (nextProps.fitBounds) {
        const { fitBounds } = this.props;

        const didFitBoundsUpdate =
          fitBounds !== nextProps.fitBounds && // Check for reference equality
          (nextProps.fitBounds.length !== (fitBounds && fitBounds.length) || // Added element
          !!fitBounds.filter((c, i) => {
            // Check for equality
            const nc = nextProps.fitBounds && nextProps.fitBounds[i];
            return c[0] !== (nc && nc[0]) || c[1] !== (nc && nc[1]);
          })[0]);

        if (
          didFitBoundsUpdate ||
          !isEqual(this.props.fitBoundsOptions, nextProps.fitBoundsOptions)
        ) {
          map.fitBounds(nextProps.fitBounds, nextProps.fitBoundsOptions);
        }
      }

This would skip all other equality checks if reference equality is true.

[haysclark]

@jniebuhr Should this issue be closed? The current code in production looks almost exactly like your snippet, less the "()" around the // Added element block of code. https://github.com/alex3165/react-mapbox-gl/blob/master/src/map.tsx#L325-L343

[jniebuhr]

@haysclark No, there's still a very important difference. In line 329, you use || instead of &&. So the boolean statement basically says "If reference equality is given, check if contents are different" which cannot ever happen, thus rendering everything after || useless. Also it'll say "If reference equality is not given, bounds have changed".

My version with the && says "If reference equality is given, bounds did not update. If reference equality is not given, check if contents updated."

[haysclark]

@jniebuhr Thanks for clarifying! I should have pasted the code into my project and looked at the Diff.

While I am not a maintainer, I did try your code and noticed that your code snippet fails to pass the linter and thus the test suite.

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.755s, estimated 1s
Ran all test suites related to changed files.

 FAIL  src/__tests__/map.test.tsx
  ● Test suite failed to run

    TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
    src/map.tsx:336:15 - error TS2532: Object is possibly 'undefined'.

    336             !!fitBounds.filter((c, i) => {
                      ~~~~~~~~~

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.585s
Ran all test suites related to changed files.

Here is your code with a _fitBounds_ check which will pass the linter and the test suite:

if (nextProps.fitBounds) {
        const { fitBounds } = this.props;

        const didFitBoundsUpdate =
          fitBounds !== nextProps.fitBounds && // Check for reference equality
          (nextProps.fitBounds.length !== (fitBounds && fitBounds.length) || // Added element
            fitBounds && !!fitBounds.filter((c, i) => {
              // Check for equality
              const nc = nextProps.fitBounds && nextProps.fitBounds[i];
              return c[0] !== (nc && nc[0]) || c[1] !== (nc && nc[1]);
            })[0]);

        if (
          didFitBoundsUpdate ||
          !isEqual(this.props.fitBoundsOptions, nextProps.fitBoundsOptions)
        ) {
          map.fitBounds(nextProps.fitBounds, nextProps.fitBoundsOptions);
        }
      }

However, It's seems like it would be best to author a test that exposed the issue with the original code, and then update the solution. 😄

[henrinormak]

Any updates on this? Quite an annoying bug, as it causes the map to move around even if the contents of the bounds is equal to the previous one (something that can happen quite easily even if reference equality does not hold).

[alex3165]

Hello everyone, I agree this logic doesn't really make sense, in the end i would propose this fix: https://github.com/alex3165/react-mapbox-gl/pull/788/files but I would like to have everyone's opinion. With this shouldFitBoundUpdate logic the only way to deliberately make a fitBounds update is to break the reference of the top level fitbounds array.

[jniebuhr]

@alex3165 I think your proposed fix has the same problem as the old code. Reference equality should not matter. The only thing we can use it for is to skip the content check.

A lot of time people will pass a function into fitBounds that generates some bounds from a state. Or they’ll build the fitBounds array in their render method. Whenever you do that, it zooms around.

What you should do is just compare the contents of the array. (You may skip comparison if reference equality is given, as in my proposed code)

[alex3165]

Ok I though using ref equality made sense as i was thinking about zoom and center where it might be updated in mapbox-gl context but then make the React state of the user wrong which is why I build this whole ref check logic for zoom and center but in the case of fitbounds I guess it would not be updated within mapbox-gl context since it is only driven by the fitBounds prop so indeed content check of the fitbounds would make sense, I will update the PR.

Edit: I updated the Pull request

[jniebuhr]

Yes, that looks a lot better. It just might fail with some errors now if one of the arrays is malformed.