File viewer: Support 'hashchange' navigation events

[jankeromnes]

STR

1. git clone https://github.com/mozilla/firefox-code-coverage-frontend/ front && cd front
2. yarn install && yarn start
3. Visit http://localhost:5000/#/file?revision=980d4dc65afc&path=mfbt/Assertions.cpp
4. Edit URL to http://localhost:5000/#/file?revision=980d4dc65afc&path=mfbt/ChaosMode.cpp and hit Enter

Expected result

• The page should load + show the coverage data of ChaosMode.cpp

Actual result

• Nothing happens (you need to hit Cmd + R to force a page reload and get the correct data)

The fix

This pull request fixes this by resetting the state when getting a 'hashchange' event.

@armenzg please take a look.

---

This change is 

[armenzg]

This is a problem caused for using the hash routing rather than the browser routing. We use hash routing because Kyle ships the UI to one of his dashboards on S3.

I wonder if this change would be better suited at a higher point in the hierarchy of the tree since the diff viewer also suffers from this issue.

I wonder if this is better written with a higher order component and we can use it on routes.jsx to wrap DiffViewerContainer and FileViewerContainer.

Or perhaps withRouter is all we would need:
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/api/withRouter.md

A side note, I wonder if this:

<Route
      path="/file"
      component={FileViewerContainer}
/>

should have been written as this:

<Route
      path="/file"
      render={({ location }) => (
        <FileViewerContainer
            revision={location.search.revision}
            path={location.search.path}
        />
      )}
      component={FileViewerContainer}
/>

This would simplify parseQueryParams() to no have to set the state as it currently does.

[jankeromnes]

Thank you @armenzg for the review!

But after doing some research, it seems that React's router actually does update on hashchange events, except neither componentDidMount nor setState will be called on its components, but componentWillReceiveProps: remix-run/react-router#292

As this is already done elsewhere in the code, I'll change my fix to use componentWillReceiveProps instead.

[eliperelman]

Could it be that you are relying on query params to change the history when they are parsed separately from the hash?

[jankeromnes]

Apparently componentWillReceiveProps should not be used to trigger network requests, and we should use componentDidUpdate instead (with a condition to only send requests when the situation actually changes):

https://medium.com/@nimelrian/as-of-react-16-you-should-use-componentwillreceiveprops-only-to-update-state-synchronously-as-in-a9d66457c510

But maybe there is an even cleaner way to do this, by not copying props into state, but using props directly (implies parsing query params in render). Will update accordingly.

[jankeromnes]

Could it be that you are relying on query params to change the history when they are parsed separately from the hash?

I don't think we use query parameters in addition to the hash (yet).

[eliperelman]

Correct. In React 16 componentWillReceiveProps is deprecated. Use componentDidMount.

I don't think we use query parameters in addition to the hash (yet).

According to these links, you are changing the query string:

http://localhost:5000/#/file?revision=980d4dc65afc&path=mfbt/Assertions.cpp
http://localhost:5000/#/file?revision=980d4dc65afc&path=mfbt/ChaosMode.cpp

You can use the render along with a URLSearchParams to get an easy object of query string parameters.

[jankeromnes]

Correct. In React 16 componentWillReceiveProps is deprecated. Use componentDidMount.

I think you meant "Use componentDidUpdate" instead, because componentDidMount never gets called a second time even when props change.

[eliperelman]

Yep, my mistake!

[jankeromnes]

But maybe there is an even cleaner way to do this, by not copying props into state, but using props directly (implies parsing query params in render). Will update accordingly.

Well, we still need to asynchronously fetch new data whenever revision or path change, so I still went with the componentDidUpdate solution (gated by a check that props.location.search actually changed).

[jankeromnes]

@armenzg Please take another look.

[eliperelman]

Setting anything in state to undefined is synonymous with the field not being changed. You will want to set these to null.

[jankeromnes]

Actually, this line does successfully change state.coverage to undefined in the next render call, properly displaying that this information is pending again.

[armenzg]

Address the question, fix the linting issues (see Travis) and feel free to land if @eliperelman has no objections.

I wonder how linting issues could have been introduced with lint-staged and husky:
https://github.com/mozilla/firefox-code-coverage-frontend/blob/master/package.json#L20

[armenzg]

Why do we make two calls to setState() instead of one?

[jankeromnes]

The first call resets coverage and parsedFile (to clear old data), and the second call updates appError, revision and path according to the new URL.

We could do both in one call, but that would imply hacking the object returned by this.parseQueryParams() to add the two coverage: undefined and parsedFile: undefined entries, which would be even more verbose.

[jankeromnes]

and feel free to land if @eliperelman has no objections.

@eliperelman, please see my reply to your comment. Setting state entries to undefined does reset them, and furthermore using undefined already seems to be the convention in this repository (search for "undefined" in src/containers/fileViewer.jsx and src/components/fileViewer.jsx).

Please let me know if you feel very strongly about null, in which case I'll add a commit to update the entire repository.

I wonder how linting issues could have been introduced with lint-staged and husky:
https://github.com/mozilla/firefox-code-coverage-frontend/blob/master/package.json#L20

That's my fault. I really hate commit hooks, because they totally break my flow (I want commit to happen instantly, even for WIP / temporary / experimental commits that may have lint issues) so my muscle memory now adds --no-verify every time I commit.

EDIT: And also, #194 makes linting perma-fail on my setup (because I use Node 10).

[jankeromnes]

@armenzg the two Travis failures are caused by the lint rule react/no-did-update-set-state, which forbids setState in componentDidUpdate to prevent infinite loops:

error: Do not use setState in componentDidUpdate (react/no-did-update-set-state)

However, the consensus seems to be that this is OK, as long as you wrap your setState in a sensible condition (see this stackoverflow discussion). So I'll just silence this warning.

From today on IRC: 15:41:03 ~Eli> janx: if armen is happy, ship it