Skip to content

Using redux-first-router within the application with the base URL different from '/'? #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
re6exp opened this issue Sep 8, 2017 · 52 comments

Comments

@re6exp
Copy link
Contributor

re6exp commented Sep 8, 2017

How to use this component with history/createMemoryHistory at server side, if we have base URL different from '/'?
This code doesn't work:
const history = createHistory({ initialEntries: [req.path] })
This doesn't work too:
const history = createHistory({ initialEntries: ['<publicUrl>' +req.path] })

@faceyspacey
Copy link
Owner

So u want all URLs to start with /basename? Correct.

This is a feature I plan to add this weekend in fact. Can you do a test for me? Look at the history package; you will see they accept an argument for this. Can you verify RFR fully functions when using their 'basename' arg.

https://www.npmjs.com/package/history

@re6exp
Copy link
Contributor Author

re6exp commented Sep 8, 2017

At the client side everithing OK - basename with createBrowserHistory does the job. The issue appears while attempting to render some page (not root one) at the server side.
Tomorrow I'm going to continue with the thing, I'll let you know what I'll get.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 10, 2017

My experiments I'm doing are on your redux-first-router-demo.

I have spring cloud infrastructure built on base netflix components, and modified redux-first-router-demo that can register to the cloud. So I need to change base url to properly work with netflix zuul (gateway app).

At client side as I said above everything is OK - after basename initialized with appropriate path in createBrowserHistory. So this part has no problems with base url/app name, and it works as expected.

At the server side (javascript app - demo app based on your redux-first-router-demo), createMemoryHistory is used. createMemoryHistorydoesn't support basename property, as you've really done there before through initialEntries initialized by request#path.

The url mismatching appears when I attempt to refresh browser while current url differs from '/', for example, '/list/db-graphql'. I'm looking for why, but at the moment I have no idea yet.

But there is interesting thing I've observed from time to time while I'm playing with basename/initialEntries/webpack-publicPath etc: whatever I've trying to change, there is one link reported by server (from logs: REQUESTED PATH) which url doesn't changed anyway: hot-update.json url does not based on url I change, it is always '/static/'. Even using webpack-hot-middleware/client?path=/<basename>/__webpack_hmr and hotUpdateChunkFilename/hotUpdateMainFilename tricks. But url for __webpack_hmr is changed accordingly to my edits...

Now I am trying to find the cause of error while using ssr.

@faceyspacey
Copy link
Owner

Can you send me a repo?

@re6exp
Copy link
Contributor Author

re6exp commented Sep 10, 2017

Here it is.. But there is nothing special except for eureka connection and publicUrl/basename changes...

@re6exp
Copy link
Contributor Author

re6exp commented Sep 10, 2017

You can cut eureka connection, it does not effect on something.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 11, 2017

So, I found the solution. It has no relation to redux-first-router - I've change settings on my cloud part and now it works perfect with redux-first-router!

I enlarge ribbon and eureka timeouts and that was enough for fix this strange behavior.

Thank you James for fast reaction and my best wishes!

@re6exp re6exp closed this as completed Sep 11, 2017
@faceyspacey
Copy link
Owner

That's excellent. So just to confirm, basenames provided to both histories works perfectly and there are no changes RFR has to make?

  • What about with the Link and NavLink components? All good there?

  • What about if you call history.push directly?

  • what paths show in the location state and action.meta.location.pathbame? The basename removed?

I'm still gonna look at your repo later today. I need to verify absolutely every use case works. This is exciting news though.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 12, 2017

The comparison is here.

  1. The basename property should be added only when using createBrowserHistory. Here, initialization of the history looks like
const history = createHistory({ basename: '/admin' })

and this works perfect.

  1. The createMemoryHistory does not support basename property, but the code
import createHistory from 'history/createMemoryHistory'
...
const history = createHistory({ initialEntries: [req.path] })

does the job properly whithout any changes.

  1. Link and NavLink as well as hyperlinks in Home.js, Sidebar.js and routeMaps.js do not need to be changed. They work as they are.

  2. In the config files, I add/change module.exports.output.publicPath to appropriate path.

5.The basename do removed in action.meta.location.pathname, indeed.

Now I'm trying to call history#push directly.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 12, 2017

If I insert a string history.push('/list/react-redux') into server/configureStore.js and refresh window (to call ssr) while the address in browser is http://<host>:<port>/<basename> (public url, corresponding to '/', do not ), browser shows the appropriate view. So, as I understand, it works:


// server/configureStore.js

...

export default async (req, res) => {
  const jwToken = req.cookies.jwToken // see server/index.js to change jwToken
  const preLoadedState = { jwToken } // onBeforeChange will authenticate using this

  console.log(`req.path:${req.path}`)

  const history = createHistory({
    initialEntries: [req.path]
  })

  history.push('/list/react-redux')

  const { store, thunk } = configureStore(history, preLoadedState)
  ...

Hooray!

@faceyspacey
Copy link
Owner

So basically the history package's memory history does not support the basename option and that's the problem. Compare each by searching for the word basename and you will see:

The plan is now to fork the packages and add the feature. I'll do a PR, but i did one for something else and I dont think the response time will be as fast as we need. We're also going to import the history package manually when RFR becomes Rudy (well, in the @next branch too). So you will no longer have to provide this package, and we'll pass along all relevant options. These sort of things are ultimately what we gotta do to control our own destiny.

Any help would appreciated. If you wanted to create the fork of memory history (and you probably should create a PR to history first, just to cover all bases), that would be much appreciated. You should be able to just copy how the basename option is used and applied in createBrowserHistory.

That should actually 100% solve your problem if client-side is working.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

No, James! May be my english mislead you... There is no need to fork history package! Memory history, as it initialized in demo app, works perfect as is. Basename property need to be specified only in browser history. The main problem of mine was incorrect configuration of my backoffice. I increase timeouts, I edit several properties and now everything are working. As I understand, when memory history is initialized, base URL is added somewhere inside of history offal. Now all work. See the clone code of redux-first-router-demo link above. All parts, client and server side, work at the moment.
I do not know, may be you have some reasons to modify memory history, but for demo to work there is no need to fork/edit memory history.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

May be, memory history works correct without basename provided because of corrected publicUrl in webpack config.

@faceyspacey
Copy link
Owner

I see. The only thing is the URLs are not in fact correctly embedded in the HTML, which isn't good for SEO. They dispatch the correct actions when clicked, the history api will push the correct URL with the basename in the browser, and in fact the markup checksums match when going from the client to server, but if you inspect the page or view the source the URLs are not correct.

So search engines will pick up the incorrect URLs, even though the app knows how to handle them.

I know what needs to be done. I've finally started on this now.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

Ah, I got it! This thing I didn't take into account...

You know, I'm not very experienced in SEO, postponing SEO to later, but if I can help you with smth, please let me know! We all are interested in SEO to work properly. May be, I can help you with some tasks, I'll be glad to help you!

You know, I'm more experienced in server-side java world, but I'm always happy to new knowledge.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

Now I'm looking at history package... This one, right? As it follows from the post.

@faceyspacey
Copy link
Owner

Any help is much appreciated. If you can help me clone the basename stuff into memory history, I can make the links prepend the pathname. I'm going to bed now but will pick up in the morning.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

OK.
Now I'm doing this - in Moscow it is only 2 p.m.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 13, 2017

If my understanding of logic of the browser storage is correct, publicUrl/basename should be stripped from the whole url while it is operated on and it can be reconstructed by createHref method. See here.
Below call ssr with modified memory storage:

req.path:/list/db-graphql
Warning: You are attempting to use a basename on a page whose URL path does not begin with the basename. Expected path "/list/db-graphql" to begin with "/admin".
basename in deal/list/db-graphql
-- CLIENT HISTORY: {"length":1,"action":"POP","location":{"pathname":"/list/db-graphql","search":"","hash":"","key":"ljc8k7"},"index":0,"entries":[{"pathname":"/list/db-graphql","search":"","hash":"","key":"ljc8k7"}]}
-- CLIENT history.createHref(location): "/admin/list/db-graphql"
-- SERVER HISTORY: {"length":1,"action":"POP","location":{"pathname":"/list/db-graphql","search":"","hash":"","key":"ljc8k7"},"index":0,"entries":[{"pathname":"/list/db-graphql","search":"","hash":"","key":"ljc8k7"}]}
-- SERVER history.createHref(location): "/admin/list/db-graphql"
location:{"pathname":"/list/db-graphql","type":"LIST","payload":{"category":"db-graphql"},"prev":{"pathname":"","type":"","payload":{}},"kind":"load","hasSSR":true,"routesMap":{"HOME":"/","LIST":{"path":"/list/:category"},"VIDEO":{"path":"/video/:slug"},"PLAY":{"path":"/video/:slug/play"},"LOGIN":"/login","ADMIN":{"path":"/admin","role":"admin"}}}
app.get('/api/videos/:category', async (req, res), db-graphql
REQUESTED PATH: /list/db-graphql
CHUNK NAMES [ 'List' ]

Here is our long-suffering demo. The history package inserted in demo's node_modules like described here - using npm link.

Notice:
_/admin is a basename. There is also route 'admin' as part of '/admin/admin'.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 14, 2017

We need to pass basename to redux-first-router#connectRoutes, may be add basename property to Location type in order to yield the correct url in redux-first-router-link#Link.

We can get this corrected url by concatenating basename and url somewhere ( toUrl(to, routesMap) ) or by using "interface method" from history package (createHref method). It appears in all history types: createBrowserHistory, createMemoryHistory, createHashHistory.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 15, 2017

Yestarday I learned how history and redux-first-router are working. The principally new thing for me was flow. Never mind...
Now I'm writing delegate to history package. If they include somewhen basename to createMemoryHistory, it can be removed easily.
After it will be ready, I can pull my changes to your repository.

@faceyspacey
Copy link
Owner

You around? ...sorry, i got bogged down with client work. I'm diving into this now.

@faceyspacey
Copy link
Owner

The latest code is in the next branch:

https://github.com/faceyspacey/redux-first-router/tree/next

...Also, I'm already monkey-patching the history package, but im gonna remove that, and just include the history package directly, and then just require users to pass req.path or [req.path] to connectRoutes instead of your history package. It will be a breaking change when RFR becomes Rudy. ..but for now we'll allow passing in history as a third option, except it won't work with new features like basename and confirmLeave.

...I'm going through all your code now.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

I understand, James, that you write this in your spare time, thank you for that!

We did similar job in parallel...
I wrote monkey code too. I wrap history in redux-first-router. I did not commit yet, because I was not sure what kind of the way is the best... I waked up just now. Computer is booting...

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

basename now is not what it should be - it is full path, that must be in href of a.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Trying to commit, I got test errors... I need some time for fix them.
Commited, tests skipped for fast code review (tests don't work now).

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

See my wrapper.

In demo, src/index.js:

import React from 'react'
import ReactDOM from 'react-dom'
import { Provider } from 'react-redux'
// import createHistory from 'history/createBrowserHistory'
import { createBrowserHistory as createHistory } from 'redux-first-router'
...
const history = createHistory({
  basename: '/admin'
})
 ...

server/configureStore.js:

// import createHistory from 'history/createMemoryHistory'
import { createMemoryHistory as createHistory, NOT_FOUND } from 'redux-first-router'
// import { NOT_FOUND } from 'redux-first-router'
import configureStore from '../src/configureStore'

export default async (req, res) => {
  ...
  const history = createHistory({
    initialEntries: [req.path],
    basename: '/admin'
  })
  ...

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

So, for user, changes are only to change history source to wrapper and add basename to createMemoryHistory.
But the result basename property, I repeat, is not basename, it is fullpath. I should have to rename it,

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

And, indeed I don't use first attempt to edit history package in this wrapper. It is alone, and should wrap original history package.
I've made many dirty things in code... :(((

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Now I'm going to make full url useful. Some reading before...
I have commited both packages to my repo to show you what I've done.
See them: demo + redux-first-router

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Now I think, may be user should initialize original history package themself, but pass it to our wrapper?.. With, in case of memoryHistory, an additional basename property.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Also, I'm already monkey-patching the history package, but im gonna remove that, and just include the history package directly, and then just require users to pass req.path or [req.path] to connectRoutes instead of your history package. It will be a breaking change when RFR becomes Rudy. ..but for now we'll allow passing in history as a third option, except it won't work with new features like basename and confirmLeave.

May be I'm wrong, but I cant see that changes...

@faceyspacey
Copy link
Owner

Ok, sorry about that. I got pulled away again. I'm here and have no distractions.

So what I meant by monkey patching is this:

https://github.com/faceyspacey/redux-first-router/blob/next/src/pure-utils/confirmLeave.js#L118

I had to do it for the confirmLeave feature.

Now instead we are going to make the package rudy-history. I.e. since RFR will be renamed history. I'm gonna take your fork of history which I analyzed earlier (which seemed perfect) and make the new package. I hope you don't mind (i'll keep the commits you made so you got credit for it). Basically this is something I've planned for a long time as I knew eventually RFR/Rudy would have to customize that package too much. And ultimately I/RFR/Rudy has to own that package to insure it's easily maintained and up to date.

...So I'm going to make sure tests pass on rudy-history for the code you wrote. Then I'm gonna publish it (like in an hour). Then I'm gonna give connectRoutes a basename and confirmLeave option. Then Rudy internally is gonna create your history. Then we will never receive history as an argument anymore. We will only accept a path or an array of paths. And then within connectRoutes we will create the history package. We will also detect whether you are in the browser, and determine whether you need a memory history or browser history :). The downside is that we now gotta ship both parts of the history package, but memoryHistory is very small and it's a small cost.

After that I'll remove my current monkey patching technique. By the "money patching" is almost a technical term, and it means when you overwrite functions in another library. It's best to avoid doing that, especially since it can have unpredictable side-effects for advanced users using npm-link and webpack/babel/node alias capabilities. Sometimes the wrong package is included, and any code that monkey-patched it in fact changed 1 of 2 packages being used from node_modules, and the result is perhaps that monkey-patching did not work. So by "owning" the history package we can circumvent all the problems and do this correctly.

In short, u did all the primary work in your commits yesterday to fork history and add basename to createMemoryHistory. Thanks very much for this! After the basic wiring is setup to use this, then I just gotta change the link package to incorporate basenames. There may be one other such basename prepending task, but it won't be very time-consuming. It may even come in as a bug when someone else finds it, and that's fine--that's why this is in @next.

Starting for real now.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Ok!
I take pause and pick up everithing needed some later today or tomorrow to prevent doing the same things you do the better way. Please, let me know when I could take the new version (and rudy-history)!
I will test it on my backend and write you about results.
Today I am a little busy, but tomorrow I can help you if you need some help!

@faceyspacey
Copy link
Owner

faceyspacey commented Sep 16, 2017

ok, the package is live with tests and one other small new feature to maintain parity to browser history:

https://www.npmjs.com/package/rudy-history
https://github.com/faceyspacey/rudy-history

...time to incorporate it in Rudy.

@faceyspacey
Copy link
Owner

faceyspacey commented Sep 16, 2017

i decided to PR the original history package even tho we wont be using it (at least for now). gave u credit:

remix-run/history#522

@faceyspacey
Copy link
Owner

Done. It's on npm. You can get it like this:

yarn upgrade redux-first-router@rudy
yarn upgrade redux-first-router-link@rudy

you can use it like this:

client:

connectRoutes(routesMap, { basename: '/awesome' })

server:

connectRoutes(routesMap, { 
   basename: '/awesome',
   initialEntries: request.path // || [request.path], but not history
})

Notes:

  • notice there is no longer a history argument. i.e. connectRoutes takes 2 args instead of 3 now.
  • on the server there is now an option for initialEntries.
  • you dont have to strip the basename from request.path in userland. Internal code can handle it either way.
  • the link package will automatically prepend the basename to links
  • that's all folks

Implementation details to check out:
https://github.com/faceyspacey/redux-first-router/blob/rudy/src/connectRoutes.js#L135
https://github.com/faceyspacey/redux-first-router-link/blob/rudy/src/toUrl.js#L12

@faceyspacey
Copy link
Owner

I'm going to bed. let me know if there are any bugs. if there are, and u pr, make sure to pr the rudy branches on this repo and the link repo.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 16, 2017

Great news!
Thank you very much, James! Tomorrow I'm going to upgrade code. I will let you know about the results! Thanks again!

@re6exp
Copy link
Contributor Author

re6exp commented Sep 18, 2017

I have tryed it only today. It doesn't work properly under demo code. Now I'm looking for why.

@faceyspacey
Copy link
Owner

Check the package.json files and make sure ur in fact using th correct versions of both RFR packages.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 18, 2017

I clone both RFR and RFRL, change branch to rudy, link the cloned modules to the demo app. After that I rebuilt RFR and RFRL, restart demo app to ensure it works with the new version of RFR(L) and to have an ability to check the code localy later. The same results with errors.
At the moment, what I see are:

  1. First loading of arbitrary url works wrong: the home url is loading instead.
  2. It looks like inserting the basename happens not only when it needed but sometimes when do not.
  3. Chunk loading errors:
[UNIVERSAL-IMPORT] no chunk,  NotFound , found in "window.__CSS_CHUNKS__". If you are not serving CSS for this chunk, disregard this message.
...

Now I recheck whether I've made any mistakes in demo user code.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 18, 2017

I've made fixes. The problem based on absent basename striping when not action is using.
When I put everything to github, I will write you immediately.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 18, 2017

And, if it is not already done, I'll check the new code and the fixes on demo app in both modes, with and without basename. I hope I'll finish with it tomorrow. When it is completed, I'll write you.

@faceyspacey
Copy link
Owner

look in forward to seein it. i made tests, but i didnt apply it to a demo.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 19, 2017

Oh, it's ok! You did a great job! You write it in your spare time, instead of "enjoy life as it is", not only for yourself, doing it free of charge, for community. So, it is not bad that community should do a little job for themself. :)

@faceyspacey
Copy link
Owner

Well you did! You made this one super easy for me to finalize. Thank you, not me!

@re6exp
Copy link
Contributor Author

re6exp commented Sep 19, 2017

You overestimate my contributions... Anyway, thank you for the warm words!

Now I see test errors (router and link too), so, before commit, I should fix them.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 19, 2017

James, I created the pull requests, see below:

  1. history
    Updated rudy-history. Added two methods to export: hasBasename and stripBasename. Tests are OK without any changes.
  2. redux-first-router
    Updated redux-first-router#rudy. Added basename stripping to get the correct matching between map and URL. Also I push above the option initialization (from side effects part) to prevent it being uninitialized between the creation of connectRoute and the first initial action dispatching. Otherwise getOption returns undefined when the first route do its job. The original history removed from deps completely.
    Notice here:
    test snapsot 'nestHistory formats simplified history object for action + state' was changed by me because all enrties have real value in key property after updated to our rudy-history. Please make sure I was right when I change snapshot, may be, I don't understand smth...
  3. redux-first-router-link
    Updated redux-first-router-link#rudy. Stripping basename, too. The original history removed from deps. Tests are fixed too.
  4. All I worked on were linked with help npm link command.
  5. Tomorrow I'm going to fix original redux-first-router-demo.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 20, 2017

And, after/if you merge my pulls, we should change versions in package.json in all our depended on each other packages.

@re6exp
Copy link
Contributor Author

re6exp commented Sep 20, 2017

Demo app has been updated.
See my pull.

@BrendanFDMoore
Copy link

Just wanted to say thanks for this feature & discussion @faceyspacey & @re6exp - tons of good work here, and the documentation/history (ah-hyuk) to back it up.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants