Migration to redux simple router and async-props by sars · Pull Request #833 · erikras/react-redux-universal-hot-example

sars · 2016-01-15T15:07:07Z

No description provided.

sars · 2016-01-15T18:30:33Z

ryanflorence/async-props#42

quicksnap · 2016-01-15T19:37:31Z

Looks like the build is failing..

sars · 2016-01-15T21:12:30Z

@quicksnap Do you mean npm run build ?
It works fine for me...

quicksnap · 2016-01-15T21:44:37Z

The Travis builds are failing, that is. They're at the bottom of this comment thread.

sars · 2016-01-15T22:49:53Z

@quicksnap , yeah, I'm going to fix it after ryanflorence/async-props#42 will be merged

bdefore · 2016-01-16T20:43:25Z

@sars that's a nice helper the resolver addition. i'm wondering if they'll be amenable to merging it.

bdefore · 2016-01-16T20:51:18Z

just had a look through this PR and it provides a nice lean update for this project to react-router 2.0 and replacing custom serverside fetching with something more tightly integrated with ongoing development there. it accomplishes much of what universal-redux does (#759) apart from updating to Babel 6, which has another proposal (#778).

i still think there's value in depending on universal-redux to easier rise with the tide of future developments and provide a configurable base for those using this as a starting point, but that conversation can be independent of merging this one. in fact, this aligns RRUHE closer to UR, and would ease the migration path.

jakegibson · 2016-01-17T07:35:29Z

line 15 of src/server.js has a reference to redux-router that needs to be removed for it to build

andresgutgon · 2016-01-17T16:59:48Z

package.json

This needs to be upgraded to 0.20.x

From react-router-bootstrap README.md:

Note: Releases from v0.20.0 onward only support React Router v2.x....

andresgutgon · 2016-01-17T18:58:45Z

I'm trying to port this PR to my code.
When doing npm install I was having problems with post install script of async-props. Opened an issue in their repo:
ryanflorence/async-props#46

To be able to run npm install I installed babel globally npm install babel@x.x.x -g

bdefore · 2016-01-17T19:01:03Z

I had a PR out to fix this ryanflorence/async-props#36

But at this point I think they want to stay out of the build tool jungle. I ended up pulling async-props source directly into universal-redux in order to support it.

sars · 2016-01-18T16:45:20Z

Have a look, guys, please
#531 (comment)
It should make your life much easier...

@erikras, @bdefore If you want, I can create for your projects...

bdefore · 2016-01-18T16:46:18Z

@sars that would be great, thanks!

sars · 2016-01-20T08:41:02Z

@quicksnap , @erikras , going to redo this pr with redux-asyc-connect today. it should be good

sars · 2016-01-20T09:19:45Z

@jakegibson , thank you for comments

line 15 of src/server.js has a reference to redux-router that needs to be removed for it to build

done

This needs to be upgraded to 0.20.x

done

🔥 remove this line

done

@quicksnap , @erikras , please, have a look.
I believe this PR really simplify this example.

I did not use all features from redux-async-connect here to keep diff as small as possible...
but i'm going to add some examples in next PR

andresgutgon · 2016-01-20T09:23:37Z

💪 waiting for it. I'll try it on my project

sars · 2016-01-20T09:30:40Z

there is some problem with test:

20 01 2016 11:22:09.969:ERROR [karma]: { [Error: no such file or directory]
  code: 'ENOENT',
  errno: 34,
  message: 'no such file or directory',
  path: '/_karma_webpack_/tests.webpack.js' }
Error: no such file or directory
    at MemoryFileSystem.readFileSync (/Users/rodik/RubymineProjects/react-redux-universal-hot-example/node_modules/memory-fs/lib/MemoryFileSystem.js:114:10)
    at MemoryFileSystem.readFile (/Users/rodik/RubymineProjects/react-redux-universal-hot-example/node_modules/memory-fs/lib/MemoryFileSystem.js:297:21)
    at doRead (/Users/rodik/RubymineProjects/react-redux-universal-hot-example/node_modules/karma-webpack/index.js:156:26)
    at Plugin.readFile (/Users/rodik/RubymineProjects/react-redux-universal-hot-example/node_modules/karma-webpack/index.js:160:3)

but i don't see how this is related to PR
and cannot understand hot to fix it....

lemonCMS · 2016-01-20T09:52:53Z

@sars, great PR, just one question. When the widget fails to load (33%), the widget page keeps waiting for a response and will never load. Do you have any solution for that?

sars · 2016-01-20T10:09:47Z

Oh yes, i fixed tests :)

sars · 2016-01-20T10:38:59Z

@lemonCMS fixed in async connect 0.1.4
fixed in this PR too
thanks

erikras · 2016-01-20T10:49:31Z

This is gorgeous. 👍 Great work, @sars!

erikras · 2016-01-20T10:52:42Z

I noticed you removed scrollableHistory. Does this not have the problem that was solving?

sars · 2016-01-20T11:02:18Z

@erikras thanks for comments

I don't completely understand what scroll-behavior lib does...
But anyway - it looks like it isn't compatible with react-router 2:

taion/scroll-behavior#28

lemonCMS · 2016-01-20T11:09:26Z

@sars when the route changes depending on witch scroll-behavior you have implemented, the page scroll to the top. Without this, the scroll position stay's the same.

But indeed, not yet compatible.

Better initialization of react router redux on server-side (with original url)

sars · 2016-02-11T13:25:33Z

@quicksnap Rebased
About deferred data fetching - I'm working on 1.0.0 version now. I'm going to use redial approach, i think.

FoxxMD · 2016-02-11T13:44:52Z

👍

mmahalwy · 2016-02-11T18:12:45Z

@sars sorry to do this to you, so close! I found a bug that is due to removing this line: https://github.com/erikras/react-redux-universal-hot-example/pull/833/files#diff-e6a5b42b2f7a26c840607370aed5301aL97

Now query params are not set on the location. I don't know if that's a problem with us here or a problem with react-router-redux not picking up on the location.

sars · 2016-02-11T20:30:55Z

@mmahalwy I used react-router/lib/createMemoryHistory https://github.com/erikras/react-redux-universal-hot-example/pull/833/files#diff-e6a5b42b2f7a26c840607370aed5301aR17
to create history.
And it should add query... see:
https://github.com/rackt/react-router/blob/master/modules/createMemoryHistory.js#L11

mmahalwy · 2016-02-11T21:58:01Z

@sars hmm weird, I must have had your old changes. You're right! Works perfectly :)

sars · 2016-02-13T00:02:57Z

Guys, take a look please at new version of redux-async-connect

brocoders/redux-async-connect#16 (comment)

Documentation is in progress... But it solves several significant issues: router in params, ability to define something like deferred props, etc.

several modules updated

sars · 2016-02-13T12:09:19Z

scroll-behavior works now!

farrrr · 2016-02-14T17:35:24Z

src/redux/create.js


-export default function createStore(reduxReactRouter, getRoutes, createHistory, client, data) {
-  const middleware = [createMiddleware(client), transitionMiddleware];
+export default function createStore(getRoutes, history, client, data) {


getRoutes is unnecessary...

sars · 2016-02-14T21:26:08Z

@farrrr
thanks, fixed

sars · 2016-02-14T21:27:12Z

@quicksnap , I think it looks quite stable for merging to master.... what do you think?

gabriel-miranda · 2016-02-14T23:36:10Z

I want this so bad in master branch

quicksnap · 2016-02-15T08:23:31Z

It feels stable to me. I've been using it in our product and haven't had much problems. Here goes nothing!

Migration to redux simple router and async-props

snackycracky · 2016-02-15T08:29:14Z

BAM

Sent from my Tricorder

On 15.02.2016, at 09:23, Dan Schuman notifications@github.com wrote:

Merged #833.

—
Reply to this email directly or view it on GitHub.

andresgutgon · 2016-02-15T08:33:12Z

😱

OMG 👏

anomaly44 · 2016-02-15T14:14:30Z

this is great, nice work guys! thanks!

oyeanuj · 2016-02-15T18:47:40Z

this is great works folks!

would it be possible for someone involved in the PR to write a couple of bullet points as a guide on what has changed conceptually and how (essentially, how to migrate)? I suspect a lot of people who have been using this boilerplate and have done a lot of subsequent work would want to absorb these changes, and quick explanation on the changes would be very helpful!

thanks again everyone involved with it!

andrewmclagan · 2016-02-15T22:24:00Z

@oyeanuj Agreed, feel that would help allot of people out.

janhoogeveen · 2016-02-16T09:53:00Z

Don't we need to change the requireLogin method in routes.js as well?

const requireLogin = (nextState, replaceState, cb) => {
    function checkAuth() {
      const { auth: { user }} = store.getState();
      if (!user) {
        // oops, not logged in, so can't be here!
        replaceState(null, '/');
      }
      cb();
    }

    if (!isAuthLoaded(store.getState())) {
      store.dispatch(loadAuth()).then(checkAuth);
    } else {
      checkAuth();
    }
  };

to

  const requireLogin = (nextState, replace, cb) => {
    function checkAuth() {
      const { auth: { user }} = store.getState();
      if (!user) {
        // oops, not logged in, so can't be here!
        replace('/');
      }
      cb();
    }

    if (!isAuthLoaded(store.getState())) {
      store.dispatch(loadAuth()).then(checkAuth);
    } else {
      checkAuth();
    }
  };

FoxxMD · 2016-02-16T14:13:21Z

@oyeanuj +1 for a guide. This looks much cleaner but I need some clear instructions on how to migrate these changes.

sars · 2016-02-16T19:28:33Z

Guys, I'll try to create something like migration doc, but to be honest it's not so obvious. If you look at diff from this PR, you find all changes.
I'd like to add some docs and fix existing docs in this project, because some of them became a little bit outdated.
But first it would be great to solve issue with deferred props:
#928
There was such feature in this project and now there isn't
I try to find the most convenient way to solve this and need some feedback...

oyeanuj · 2016-02-29T00:25:49Z

@sars Now that #928 is merged, any high-level explanation of changes whenever you get a chance would be super appreciated :) Thanks again for all your work!

sars · 2016-02-29T13:02:10Z

@oyeanuj will do shortly

andresgutgon mentioned this pull request Jan 16, 2016

refactor(app): redux-simple-router + universal-redux npm package #685

Closed

andresgutgon mentioned this pull request Jan 17, 2016

refactor(app): use react-fetcher for data fetching #531

Closed

andresgutgon reviewed Jan 17, 2016
View reviewed changes

quicksnap mentioned this pull request Jan 20, 2016

React simple router and AsyncProps #831

Closed

sars force-pushed the master branch from 6c5bd04 to 188a1a0 Compare January 20, 2016 09:12

sars force-pushed the master branch from 4abe08e to 188a1a0 Compare January 20, 2016 09:51

sars force-pushed the master branch from f0da304 to e327fe4 Compare February 11, 2016 13:14

Removing dependency from history module

a7a55b7

Better initialization of react router redux on server-side (with original url)

Scroll behavior

97773e5

several modules updated

farrrr reviewed Feb 14, 2016
View reviewed changes

removing unused getRoutes from createStore function

a4a2c05

quicksnap added a commit that referenced this pull request Feb 15, 2016

Merge pull request #833 from sars/master

5215f66

Migration to redux simple router and async-props

quicksnap merged commit 5215f66 into erikras:master Feb 15, 2016

bdefore mentioned this pull request Feb 16, 2016

chore(allTheThings): update to universal-redux npm module (react-router 2, redux-simple-router, babel 6) #759

Closed

andresgutgon mentioned this pull request Feb 16, 2016

Feature/migrate data fetching to redux async connect coopdevs/katuma-web#9

Merged

2 tasks

Conversation

sars commented Jan 15, 2016

Uh oh!

sars commented Jan 15, 2016

Uh oh!

quicksnap commented Jan 15, 2016

Uh oh!

sars commented Jan 15, 2016

Uh oh!

quicksnap commented Jan 15, 2016

Uh oh!

sars commented Jan 15, 2016

Uh oh!

bdefore commented Jan 16, 2016

Uh oh!

bdefore commented Jan 16, 2016

Uh oh!

jakegibson commented Jan 17, 2016

Uh oh!

andresgutgon Jan 17, 2016

Choose a reason for hiding this comment

Uh oh!

andresgutgon commented Jan 17, 2016

Uh oh!

bdefore commented Jan 17, 2016

Uh oh!

sars commented Jan 18, 2016

Uh oh!

bdefore commented Jan 18, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

andresgutgon commented Jan 20, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

lemonCMS commented Jan 20, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

erikras commented Jan 20, 2016

Uh oh!

erikras commented Jan 20, 2016

Uh oh!

sars commented Jan 20, 2016

Uh oh!

lemonCMS commented Jan 20, 2016

Uh oh!

sars commented Feb 11, 2016

Uh oh!

FoxxMD commented Feb 11, 2016

Uh oh!

mmahalwy commented Feb 11, 2016

Uh oh!

sars commented Feb 11, 2016

Uh oh!

mmahalwy commented Feb 11, 2016

Uh oh!

sars commented Feb 13, 2016

Uh oh!

sars commented Feb 13, 2016

Uh oh!

farrrr Feb 14, 2016

Choose a reason for hiding this comment

Uh oh!

sars commented Feb 14, 2016

Uh oh!

sars commented Feb 14, 2016

Uh oh!

gabriel-miranda commented Feb 14, 2016

Uh oh!

quicksnap commented Feb 15, 2016

Uh oh!

snackycracky commented Feb 15, 2016

Uh oh!

andresgutgon commented Feb 15, 2016