React 16 #104

landonreed · 2019-08-23T13:41:53Z

Update app to React 16. This also:

updates a number of other deps,
removes PropTypes imports from React, and
changes some lifecycle methods as required by migration to 16.

Note: You'll see some warnings about lifecycle methods needing to be updated, but these are for components in react-bootstrap. We're currently pinned to the latest non-beta (and non Bootstrap v4) version of the dependency, so I'm not sure there's much we can do (and it's probably not worth worrying about since these are just warnings).

fix #47

codecov-io · 2019-08-23T14:46:28Z

Codecov Report

Merging #104 into dev will increase coverage by 0.07%.
The diff coverage is 3.78%.

@@            Coverage Diff             @@
##              dev     #104      +/-   ##
==========================================
+ Coverage   10.54%   10.62%   +0.07%     
==========================================
  Files         134      134              
  Lines        6039     6079      +40     
  Branches     1748     1750       +2     
==========================================
+ Hits          637      646       +9     
- Misses       4579     4614      +35     
+ Partials      823      819       -4

Impacted Files	Coverage Δ
lib/components/mobile/results-screen.js	`0% <ø> (ø)`	⬆️
lib/actions/api.js	`44.37% <0%> (ø)`	⬆️
lib/components/narrative/default/transit-leg.js	`0% <0%> (ø)`	⬆️
lib/components/app/responsive-webapp.js	`0% <0%> (ø)`	⬆️
lib/components/narrative/default/tnc-leg.js	`0% <0%> (ø)`	⬆️
lib/components/map/zipcar-overlay.js	`0% <0%> (ø)`	⬆️
lib/components/form/date-time-modal.js	`0% <0%> (ø)`	⬆️
lib/components/narrative/leg-diagram-preview.js	`0% <0%> (ø)`	⬆️
lib/components/narrative/narrative-itineraries.js	`0% <0%> (ø)`	⬆️
lib/components/form/date-time-preview.js	`0% <0%> (ø)`	⬆️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d24ce6...b9be98d. Read the comment docs.

…flet custom components

componentWillReceiveProps is deprecated in React 16

evansiroky

Is this still a draft PR, or is it ready for review? If it's ready, please remove the Draft label.

lib/components/app/responsive-webapp.js

lib/components/form/date-time-selector.js

lib/components/map/base-map.js

This was added in an attempt to silence an error/warning during debugging the upgrade to React 16, but the fix was a red herring (something else was the matter).

evansiroky · 2019-08-26T22:16:22Z

lib/components/form/location-field.js

+    // If location is updated externally, replace value and geocoded features
+    // in internal state.
+    // TODO: This might be considered an anti-pattern. There may be a more
+    // effective way to handle this.


This is less-worse than how the geocode results are saved and accessed later. This whole component could use a good refactor.

Something for a later round of work...

evansiroky · 2019-08-26T22:25:10Z

lib/components/map/base-map.js

+  componentDidUpdate (prevProps) {
+    this._updateBounds(prevProps, this.props)
+    // Check if any overlays should be toggled due to mode change
+    this._handleQueryChange(prevProps.query, this.props.query)


This just feels like most of it should be handled elsewhere and not here. Wouldn't these methods get fired on every component update?

evansiroky · 2019-08-26T22:26:18Z

lib/components/map/leg-diagram.js

-    if (leg && this.props.leg && leg.startTime !== this.props.leg.startTime) {
+  componentDidUpdate (prevProps) {
+    const { leg } = this.props
+    if (leg && prevProps.leg && leg.startTime !== prevProps.leg.startTime) {
      this._determineCompressionFactor(this.state.width, leg)
    }


This should probably not be handled with state. Something for a future refactor.

evansiroky · 2019-08-26T22:29:25Z

lib/components/map/route-viewer-overlay.js

        (acc, ptn) => {
          return acc.concat(polyline.decode(ptn.geometry.points))
        },
        []
      )
-      this.context.map.fitBounds(allPoints)
+      this.props.leaflet.map.fitBounds(allPoints)


This feels kind of clunky. Isn't it possible for the react-leaflet map component to accept a bounds prop? If so, it'd be much better to have the bounds for the map set once where one would expect it to be set instead of like here and other places.

Yes, I agree with you on this point, but making the bounds entirely controlled is probably an issue for another day. I want to avoid changing too many things at once.

evansiroky · 2019-08-26T22:38:19Z

lib/components/narrative/narrative-routing-results.js

      this.props.setMainPanelContent(null)
    }
-    if (!this.props.error && nextProps.error) this.props.setMainPanelContent(null)
+    if (!prevProps.error && this.props.error) this.props.setMainPanelContent(null)


This all feels like it should be part of an action behind the scenes and not be called after a particular component updates.

I can add a FIXME, but I think we should be careful about changing too many things at once. This PR is intended to update React 16. Perhaps the correct approach to fixing something like this would be to add ticket this and add any thoughts on a proposed solution there if you have them

evansiroky · 2019-08-26T22:40:52Z

lib/components/viewers/route-viewer.js

  componentDidMount () {
-    // this.props.findRoute({ routeId: 'TriMet:1' })
-    // this.props.setViewedRoute({ routeId: 'TriMet:1' })
+    this.props.findRoutes()


This could result in numerous findRoutes actions being dispatched whenever the component updates. It seems the intent of this is to do this on the loading of the component, so a better spot to put this would probably be in the constructor.

It seems like the recommendation is to put ajax requests in componentDidMount rather than constructors. In any case, I'm not sure it matters that much: reactjs/react.dev#302 (comment)

Regardless, this wouldn't be dispatched whenever the component updates -- only on mount.

evansiroky · 2019-08-26T22:41:35Z

lib/components/viewers/trip-viewer.js

+  componentDidMount () {
+    const { findTrip, viewedTrip } = this.props
+    const { tripId } = viewedTrip
+    findTrip({ tripId })


Should also probably be in the constructor.

See #104 (comment)

evansiroky

The default of renaming componetWillMount/componentWillReceiveProps to componentDidUpdate doesn't seem like the right approach for a lot of cases. I think putting stuff in constructor for componentWillMount is probably a better idea. For componentWillReceiveProps, it does seem right for a few cases, but others maybe not. I think we may end up doing some more refactors on these changes in the future. I don't think we need to do all that now, but it seems like this reveals a few things that could use a future refactor:

PropTypes should be replaced with static typing as a lot of the PropTypes are so far away from what actually are the props of components.
An audit of everything that now exists in componentDidUpdate should probably occur. It feels like a lot of what's going on in these components could be handled by moving component into the redux store.
A lot of these componentDidUpdate methods have things that set the map bounds. It could get hard to track down all these individual calls to leaflet. It's not a very React-way-of-doing-things this way.

evansiroky

All right, fine. I'll approve. This all does appear to work fine and there aren't any extra calls that I can see, so good enough. While there's always room for refactors and other ideas, I'll not even recommend creating tickets just yet as the refactoring opportunities I see now may not align with the next latest priorities and pain points we have.

evansiroky · 2019-09-03T17:14:29Z

🎉 This PR is included in version 0.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

landonreed added 4 commits August 22, 2019 16:14

feat(react): bump react to v16 (from v15)

0ae847d

fix #47

refactor(react): upgrade deps and replace React/PropTypes

c176272

refactor(react-leaflet): fix custom components for react 16 update

81ae5e3

test(enzyme-adapter-react): fix import of enzyme-adapter-react-16

561afed

landonreed added 2 commits August 23, 2019 11:36

refactor(leaflet): bump leaflet to 1.5.1 and refactor other react-lea…

90c40c1

…flet custom components

refactor(deps): bump redux-thunk

532ab65

landonreed mentioned this pull request Aug 23, 2019

Upgrade to react 16 ibi-group/trimet-mod-otp#242

Merged

landonreed added 2 commits August 23, 2019 14:17

refactor(react): replace componentWillMount with componentDidMount

999ff8b

refactor(react): change componentWillReceiveProps to componentDidUpdate

c9e0f57

componentWillReceiveProps is deprecated in React 16

landonreed requested a review from evansiroky August 23, 2019 19:59

landonreed assigned evansiroky Aug 23, 2019

evansiroky requested changes Aug 26, 2019

View reviewed changes

evansiroky assigned landonreed and unassigned evansiroky Aug 26, 2019

landonreed marked this pull request as ready for review August 26, 2019 17:34

landonreed assigned evansiroky and unassigned landonreed Aug 26, 2019

landonreed requested a review from evansiroky August 26, 2019 17:34

refactor(webapp): add back routes for router

0a03d6a

evansiroky reviewed Aug 26, 2019

View reviewed changes

lib/components/app/responsive-webapp.js Outdated Show resolved Hide resolved

evansiroky reviewed Aug 26, 2019

View reviewed changes

lib/components/form/date-time-selector.js Show resolved Hide resolved

evansiroky reviewed Aug 26, 2019

View reviewed changes

lib/components/map/base-map.js Show resolved Hide resolved

landonreed added 2 commits August 26, 2019 16:17

refactor(webapp): remove no-op function arg

8922f64

This was added in an attempt to silence an error/warning during debugging the upgrade to React 16, but the fix was a red herring (something else was the matter).

refactor(date-time-selector): move browser input check into constructor

b9be98d

evansiroky reviewed Aug 26, 2019

View reviewed changes

evansiroky requested changes Aug 26, 2019

View reviewed changes

evansiroky assigned landonreed and unassigned evansiroky Aug 26, 2019

landonreed assigned evansiroky and unassigned landonreed Aug 28, 2019

evansiroky approved these changes Aug 30, 2019

View reviewed changes

evansiroky assigned landonreed and unassigned evansiroky Aug 30, 2019

landonreed merged commit b17ab42 into dev Sep 3, 2019

landonreed deleted the react16 branch September 3, 2019 14:38

landonreed mentioned this pull request Sep 3, 2019

Minor release: React 16 #105

Merged

evansiroky added the released label Sep 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React 16 #104

React 16 #104

landonreed commented Aug 23, 2019 •

edited

Loading

codecov-io commented Aug 23, 2019 •

edited

Loading

evansiroky left a comment

evansiroky Aug 26, 2019

evansiroky Aug 26, 2019

evansiroky Aug 26, 2019

evansiroky Aug 26, 2019

evansiroky Aug 26, 2019

landonreed Aug 28, 2019

evansiroky Aug 26, 2019

landonreed Aug 28, 2019

evansiroky Aug 26, 2019

landonreed Aug 27, 2019

evansiroky Aug 26, 2019

landonreed Aug 27, 2019

evansiroky left a comment

evansiroky left a comment

evansiroky commented Sep 3, 2019

React 16 #104

React 16 #104

Conversation

landonreed commented Aug 23, 2019 • edited Loading

codecov-io commented Aug 23, 2019 • edited Loading

Codecov Report

evansiroky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

evansiroky commented Sep 3, 2019

landonreed commented Aug 23, 2019 •

edited

Loading

codecov-io commented Aug 23, 2019 •

edited

Loading