Implement React [email protected] #393

TobiahRex · 2017-03-16T06:49:45Z

Hi Cory,

Here's your hot fresh PR. :)

I've implemented [email protected] under branch "[email protected]/tobiahrex".

CHANGE NOTES:

index.js:
- Implemented per the docs. However I felt it cleaner to keep the Route tree out of the index.js file and in it's own component - App.js
App.js: Everything that was in "routes.js" is now converted (per my understanding of rrV4) and implemented here.
- "IndexLink " is no longer an option. However "NavLink" with attr: "activeStyle" is.
- "Switch" is for the NotFoundPage to catch anything not matched in the route tree, otherwise it's always mounted.
- Each of the above mentioned comps are from react-router-dom since they're no longer a part of the rrV3 API.
configureStore.js: nothing outside of the docs except...
- export const history = createHistory(); So the docs had the createStore() invoked in what is our "index.js" which I'm not a fan of. Since we want the same "history" instance @ both the configureStore.js & index.js to pass in as "ConnectedRouter history={history}", I thought it best to do this.
About, HomePage, NotFoundPage:
- Added preceding "/" to each name.
- Changed import source to "react-router-dom".
package.json:
- npm i history --save-dev
- npm i react-router
- [email protected] is a package dependency so it's not explicitly installed.
- npm i react-router-redux@next --save

Thanks for the suggestions to contribute and being open. Let me know if I can do/change anything.

Respectfully,

-Toby

TobiahRex · 2017-03-16T08:07:56Z

Anyone have any clues on getting those checks to pass?

nickytonline · 2017-03-16T09:13:18Z

@TobiahRex, as the logs state there were some network issues. I know that
Yarn was down yesterday for some reason. Probably relaunch the builds to see if you still get the issue. Also, please add the new react router packages to the yarn.lock file. See https://yarnpkg.com/en/docs/yarn-lock. You can add it via yarn add -E

mikedevita · 2017-03-16T15:47:00Z

Thanks for adding the spacing around the { and } that always bugged me lol.

TobiahRex · 2017-03-16T16:08:10Z

@nickytonline Good Feedback! Saw the logs but didn't want to assume without peer input since I have little exposure to yarn. The node version change log entry also made me double check my own node version and alas I was out of date (since updated).

Also, copy all @ yarn.lock file. Will add react router package changes and re-commit ASAP.

break...

@mikedevita haha yeah I'm a stickler for that stuff. All thanks to eslint w/airbnb style guide extension.

TobiahRex · 2017-03-16T16:29:14Z

@ALL, apologies for the typos...

"reach" = each*

"npm i react-router --save" =
npm i react-router-dom* --save

[email protected] is a package dependency so it's not explicitly installed

oshalygin · 2017-03-16T18:56:14Z

src/store/configureStore.js

@@ -30,6 +36,9 @@ function configureStoreDev(initialState) {
    thunk,
  ];

+  const reactRouterMiddleware = routerMiddleware(history);


I wouldn't push to the array, instead declare the array below all this and just include it inline.

@oshalygin
Fair enough. I'll change it.

oshalygin · 2017-03-16T18:57:14Z

package.json

-    "react-router": "3.0.0",
-    "react-router-redux": "4.0.7",
+    "react-router-dom": "4.0.0",
+    "react-router-redux": "5.0.0-alpha.3",


This feels a bit rough to be using an alpha package. Many of us are using this library for projects and these things will inevitably break into beta+

Agreed. I'd like to avoid taking on alpha/beta dependencies.

Ahh didn't see this one here still. @TobiahRex, can the RR4 PR use a stable version of react-router-redux?

How about replace react-router-redux (which is still in alpha) with connected-react-router ?

coryhouse · 2017-03-16T20:08:37Z

Thanks so much for the PR! 👍 💯 I agree that we need to wait until all deps are out of alpha. But this PR is an excellent foundation once react-router-redux goes 5 final releases.

nickytonline

@TobiahRex as mentioned in the comments already and also as @coryhouse said, let's wait until everything is out of alpha for dependencies.

TobiahRex · 2017-03-17T17:58:06Z

@nickytonline & @coryhouse Understood. Many thanks for the feedback.

kwelch · 2017-03-20T12:45:50Z

What is the need/use-case for react-router-redux? Why include it?

It understand it adds the location to the store, but what benefit does that have? Does it still warrant being included in slingshot?

The only valid case I have for this at this point is redux dev tools time travel.

I ask not to try and push this through, but more so to ensure we asked and answered the question.

This was something I chose to remove when I move our company projects to RRv4.

TobiahRex · 2017-03-21T11:37:41Z

@kwelch Great question. Outside of a dev environment, i'm not sure there's a benefit of react-router-redux.

The guys' description @ it's npm page even says...

This library is not necessary for using Redux together with React Router. You can use the two together just fine without any additional libraries. It is useful if you care about recording, persisting, and replaying user actions, using time travel. If you don't care about these features, just use Redux and React Router directly.

Perhaps the idea here with react-slingshot is based on giving devs the best possible development environment available. However, the original creators would have to speak on that to confirm.

kwelch · 2017-03-21T12:37:52Z

@coryhouse / @nickytonline How do you feel about dropping react-router-redux?

nickytonline · 2017-03-21T14:32:04Z

@kwelch, I've never used it so I'll let you and @coryhouse make that call.

…

On Tue, Mar 21, 2017 at 8:37 AM Kyle Welch ***@***.***> wrote: @coryhouse <https://github.com/coryhouse> / @nickytonline <https://github.com/nickytonline> How do you feel about dropping react-router-redux? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy2z65e-25hW7sK3jRMIe5trhi7RLOFks5rn8ShgaJpZM4Me6BQ> .

TobiahRex · 2017-03-22T05:58:26Z

@nickytonline update on upgrading the yarn.lock file...

I'm currently in Japan on a freelance job and will be for few more months. I've attempted to update the yarn.lock file with the upgrades to react-router version numbers & add react-router-dom & react-router-redux. Problem is, all 3 locations I've attempted to do this from, are giving me

"There appears to be trouble with your network connection. Retrying..."

errors from the yarn cli. After some research, (yarnpkg/yarn#668) it appears to be a common issue if the internet bandwidth is less than 2-2.5mb/s. Alas, this is my case here in Japan from all 3 locations. So, I'll continue to try, however someone stateside may have a faster time upgrading the yarn.lock file. I'll re-commit as soon as I am successful.

kwelch · 2017-03-22T14:56:25Z

Thanks @TobiahRex, as long as you have opened write access to maintainers, we should be able to push a commit to fix the lock file.

TobiahRex · 2017-04-01T16:36:25Z

FYI: I decided to a try complete removal of my node_modules/ and then run yarn command to sneak around the "Network Connection" error I mentioned previous when running yarn add -e & yarn upgrade <pkg>. The other added benefit being, yarn linking dependencies and making my post yarn, node_modules/ dir much smaller.

Result:

yarn.lock file updated react-router + friends!
yarn.lock file also updated all packages listed. <--- Problem?

nickytonline · 2017-04-01T23:42:59Z

@TobiahRex, looks like there's conflicts. Also, CI is failing with your latests commit.

TobiahRex · 2017-04-02T00:28:13Z

@nickytonline Forgot to rebase master & PR branch from the latest PR merges. In progress now. Will re-commit ASAP.

…pes error.

coveralls · 2017-08-31T06:15:42Z

Coverage decreased (-0.7%) to 94.595% when pulling 3efc625 on TobiahRex:[email protected]/tobiahrex into c4b0cf7 on coryhouse:master.

TobiahRex · 2017-08-31T06:21:39Z

@kwelch & @nickytonline Rebase complete. Couple notes...

Coveralls Coverage percentage dropped a tad (-.7%). I looked at the report and it doesn't appear to be coming from any files I personally changed (see image). I could be wrong, but it looks like it's coming from the package dependencies version upgrades. Would need another to confirm.
I believe react-router-redux hasn't upgraded their Prop Validation library to prop-types post react v15 (their v5 is still in "alpha") hence this error ... I've triple checked all our components for proper prop-types lib implementation and validation and we're 100% GTG.

kwelch · 2017-08-31T13:41:06Z

package.json

-    "react-router-redux": "4.0.8",
+    "react-router": "4.2.0",
+    "react-router-dom": "4.0.0",
+    "react-router-redux": "5.0.0-alpha.6",


Just want to ask this again before we move forward...

Are we sure we want to keep this?

cc: @coryhouse

I vote for dropping it. Easy to add if desired, and avoids an alpha dep

kwelch · 2017-08-31T13:43:14Z

The router changes look good. I am nervous that it shows so much diff in other areas. I would not have expected that after a rebase.

nickytonline · 2017-08-31T15:26:17Z

src/components/AboutPage.js

@@ -1,5 +1,5 @@
 import React from 'react';
-import {Link} from 'react-router';
+import { Link } from 'react-router-dom';


which formatting does our linter support? Spacing between {} in imports or none? I can't remember. cc: @kwelch.

Not sure, I know I prefer with spaces. I also believe prettier default is with spaces.

nickytonline · 2017-08-31T15:26:29Z

src/components/AboutPage.spec.js

@@ -1,5 +1,5 @@
 import React from 'react';
-import {shallow} from 'enzyme';
+import { shallow } from 'enzyme';


same as above.

nickytonline · 2017-08-31T15:27:12Z

src/components/FuelSavingsForm.js

-            <td><label>Date Modified</label></td>
-            <td>{fuelSavings.dateModified}</td>
-          </tr>
+            <tr>


Once again, question about the spacing changes. Is this our eslint config doing this?

@nickytonline This one looks like its from my text-editor. I use an atom package language-babel to automatically calculate JSX spacing. In this particular case, i think i nudged it over accidentally.

@TobiahRex - Would you be willing to rollback the whitespace changes in this PR to keep the diff minimal?

@nickytonline & @coryhouse After checking again, i think this is a correct addition of whitespace. The <tr> should appear as a child of <tbody> and not a sibling due to hierarchical precedence, correct? If you'd still like me to revert back to what it was before, i can do that. please confirm.

nickytonline · 2017-08-31T15:27:44Z

@kwelch, it looks like a lot of the changes are spacing changes.

kwelch · 2017-08-31T15:45:42Z

Good point, that may be prettier related. I don't think we currently are prettier friendly, but I would not mind us moving that direction. 👍

kwelch · 2017-08-31T15:47:47Z

I think this is all set with one outstanding questions see below.

OPEN QUESTION: Do we want to continue to use/support react-router-redux?
It is still in alpha.

TobiahRex · 2017-08-31T17:25:13Z

@kwelch & @coryhouse
You all may already know this, but for the sake of clarity and making an informed decision on whether to continue waiting or dropping the alpha version, i'd like to shed some light on what purpose react-router-redux 5-alpha is serving.

The ConnectedRouter component import is exclusive to react-router-redux 5-ALPHA (unavailable in react-router-redux 4). It's purpose (if you don't already know) is to keep a record of route changes in redux. https://github.com/ReactTraining/react-router/blob/master/packages/react-router-redux/modules/ConnectedRouter.js#L19
Part of rr4's new version was to cake in this redux storage & implementation of route changes automatically. The dispatching happens on actions like push via redux - if you recall previously, it was done browserHistory.push(<path>). https://github.com/ReactTraining/react-router/blob/master/packages/react-router-redux/modules/actions.js#L21

So my point is, if we remove the react-router-redux 5-ALPHA we're removing dispatchable actions such as push and redux storage automatically. It's not a requirement for rr4 usage as long as the dev implements some alternative route change api - global browser vars like window.location.

On a personal note, I find using redux's route history storage highly convenient. A couple use cases of mine:

If user has navigated to a nested route within the app, then logs into their account, this will typically force a browser reload, defaulting to the homepage post login. With something like redux-persist you can cache the redux stores route branch pre-login, then forward the user to that cached route after successful login.
Part of react-rotuer-redux default behavior is to also save things like query params automatically. This is pretty useful if you have re-direct links (from an email as an example) to a client-side page, that needs to pass a token back to the server to fetch some data. In my case i'm using GraphQL & Apollo which can automatically feed any props (like the query param token) from redux connect into a query or mutation's variables.

Overall i think the benefit is simply not having to access a global browser api (window) for extracting /mutating route data.

oshalygin · 2017-08-31T18:19:42Z

Just for reference, I tried to roll in react-router-redux and I was seeing strange bugs left and right with routing(mostly related to HMR). It could be my own codebase and this is totally anecdotal but I would love to see full support before rolling it in.

I'm open to using something that works and if people have an alternative I'd love to hear it and I'd love to try it out and let you know if it works.

Sidenote:
After upgrading to RR4 I'm not sure what I gained. I feel like its just a different mental model to manage within the application. I get that things are moving to the new way of things and leveraging Reacts lifecycle hooks instead of RR's, but I digress...

kwelch · 2017-08-31T20:29:18Z

@TobiahRex
Thank you for the added explanation.

I, like @oshalygin, found myself in a different mental model for routes after RRv4 and did not require the ability to push from within an action. I understand that I am not the only voice, I also don't want to introduce and support something that is in alpha as the purpose of slingshot is to make entry into the React ecosystem more approachable.

I am good with either approach. My main objective is to ensure this project remains stable, friendly, and approachable to all.

coryhouse · 2017-09-01T01:33:31Z

src/components/Root.js

@@ -1,6 +1,7 @@
 import React, { Component } from 'react';
 import PropTypes from 'prop-types';
 import { ConnectedRouter } from 'react-router-redux';
+console.log('%cConnectedRouter', 'background:red;', ConnectedRouter);


Looks like a console.log slipped in

coryhouse · 2017-09-01T01:45:00Z

src/store/configureStore.js

@@ -1,15 +1,20 @@
 import {createStore, compose, applyMiddleware} from 'redux';
 import reduxImmutableStateInvariant from 'redux-immutable-state-invariant';
 import thunk from 'redux-thunk';
+import createHistory from 'history/createBrowserHistory';
+// 'routerMiddleware': the new way of storing route changes with redux middlware since rrV4.


Missing "e" in middleware

coryhouse · 2017-09-01T01:47:12Z

@TobiahRex - Thanks for the thoughtful input on react-router-redux. I'm torn on whether to include it given its alpha status and the ease with which it can be added in - but I'm good with leaving it since it's already in here. I made some simple requests for changes. Thanks so much for the hard work on the PR Tobiah!

coveralls · 2017-09-01T05:17:16Z

Coverage decreased (-0.7%) to 94.595% when pulling 975fe9e on TobiahRex:[email protected]/tobiahrex into c4b0cf7 on coryhouse:master.

TobiahRex · 2017-09-01T05:20:32Z

@coryhouse - Saw and fixed issues per your feedback. Thanks for the review. Understood on being torn. Happy to contribute and I'll continue re-basing until we're ready to merge.

coryhouse · 2017-09-01T11:42:10Z

@TobiahRex - Looks like FuelSavingsForm.js still has unnecessary whitespace changes. Can you please revert?

TobiahRex · 2017-09-02T05:01:04Z

@coryhouse I left a note asking for confirmation before reverting. I think the white space you're referring to is correct due to hierarchical precedence of <tbody> to a <tr>. Please confirm.

TobiahRex · 2017-09-02T05:04:58Z

src/components/FuelSavingsForm.js

@@ -32,48 +32,48 @@ class FuelSavingsForm extends React.Component {
        <h2>Fuel Savings Analysis</h2>
        <table>
          <tbody>
-          <tr>


@nickytonline & @coryhouse
I believe the white spacing change I made here is correct. Should not a <tr> appear as a child of a <tbody> in this case and not a sibling? Please confirm you'd like it to appear as a sibling and I'll revert.

Point taken. You can leave it.

@coryhouse thanks for the confirmation.

coryhouse · 2017-09-02T11:18:42Z

I'm good w/ merging this. Any remaining reservations? @kwelch @nickytonline

kwelch · 2017-09-02T18:49:54Z

Not see anything out of the ordinary. LGTM

nickytonline · 2017-09-02T19:17:58Z

LGTM too.

…

On Sat, Sep 2, 2017 at 2:49 PM Kyle Welch ***@***.***> wrote: Not see anything out of the ordinary. LGTM — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAy2z4uKGRMAIeLV3RO_23DJ4Z5cofQFks5seaNVgaJpZM4Me6BQ> .

coryhouse · 2017-09-03T12:58:00Z

Merged! 🎉 Thanks so much for your hard work @TobiahRex!

TobiahRex · 2017-09-03T13:42:06Z

@coryhouse Thank you! Back when I first started learning React, the slingshot repo and your plural sight course was my first exposure. Really happy to contribute.

TobiahRex added 4 commits March 16, 2017 09:42

removed react-router form package.json & installed react-router-dom.

19046a0

completed index.src refactor.

24d5343

re-implemented App.js

441faac

added notes to import changes & App.js

5c3c6c2

TobiahRex changed the title ~~React [email protected]/tobiahrex~~ Implement React [email protected] Mar 16, 2017

nickytonline requested review from nickytonline and removed request for nickytonline March 16, 2017 09:15

mikedevita mentioned this pull request Mar 16, 2017

new version of React-Router? #394

Closed

oshalygin reviewed Mar 16, 2017

View reviewed changes

nickytonline requested changes Mar 17, 2017

View reviewed changes

modified middleware array @ store/configureStore.js

d3e51e2

updated yarn.lock file with fresh yarn command.

c75ff80

updated master branch.

ce00c61

cleaned up PropTypes validations - react-router-redux throwing PropTy…

3efc625

…pes error.

kwelch reviewed Aug 31, 2017

View reviewed changes

nickytonline reviewed Aug 31, 2017

View reviewed changes

coryhouse reviewed Sep 1, 2017

View reviewed changes

comment spell check & de-console on Root.js

975fe9e

TobiahRex commented Sep 2, 2017

View reviewed changes

coryhouse merged commit 134b5f9 into coryhouse:master Sep 3, 2017

BHARAT703 mentioned this pull request May 1, 2018

routing issues #562

Closed

Implement React [email protected] #393

Implement React [email protected] #393

Conversation

TobiahRex commented Mar 16, 2017 • edited Loading

TobiahRex commented Mar 16, 2017

nickytonline commented Mar 16, 2017 • edited Loading

mikedevita commented Mar 16, 2017

TobiahRex commented Mar 16, 2017 • edited Loading

TobiahRex commented Mar 16, 2017

oshalygin Mar 16, 2017 • edited Loading

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

coryhouse commented Mar 16, 2017

nickytonline left a comment

Choose a reason for hiding this comment

TobiahRex commented Mar 17, 2017

kwelch commented Mar 20, 2017

TobiahRex commented Mar 21, 2017 • edited Loading

kwelch commented Mar 21, 2017

nickytonline commented Mar 21, 2017 via email

TobiahRex commented Mar 22, 2017

kwelch commented Mar 22, 2017

TobiahRex commented Apr 1, 2017 • edited Loading

nickytonline commented Apr 1, 2017

TobiahRex commented Apr 2, 2017

coveralls commented Aug 31, 2017 • edited Loading

TobiahRex commented Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwelch commented Aug 31, 2017

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

TobiahRex Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiahRex Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

nickytonline commented Aug 31, 2017

kwelch commented Aug 31, 2017

kwelch commented Aug 31, 2017

TobiahRex commented Aug 31, 2017 • edited Loading

oshalygin commented Aug 31, 2017

kwelch commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryhouse commented Sep 1, 2017

coveralls commented Sep 1, 2017 • edited Loading

TobiahRex commented Sep 1, 2017

coryhouse commented Sep 1, 2017

TobiahRex commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryhouse commented Sep 2, 2017

kwelch commented Sep 2, 2017

nickytonline commented Sep 2, 2017 via email

coryhouse commented Sep 3, 2017

TobiahRex commented Sep 3, 2017

TobiahRex commented Mar 16, 2017 •

edited

Loading

nickytonline commented Mar 16, 2017 •

edited

Loading

TobiahRex commented Mar 16, 2017 •

edited

Loading

oshalygin Mar 16, 2017 •

edited

Loading

TobiahRex commented Mar 21, 2017 •

edited

Loading

TobiahRex commented Apr 1, 2017 •

edited

Loading

coveralls commented Aug 31, 2017 •

edited

Loading

TobiahRex commented Aug 31, 2017 •

edited

Loading

TobiahRex Aug 31, 2017 •

edited

Loading

TobiahRex Sep 2, 2017 •

edited

Loading

TobiahRex commented Aug 31, 2017 •

edited

Loading

coveralls commented Sep 1, 2017 •

edited

Loading