Update History and DevTools #89

justingreenberg · 2015-12-07T09:03:55Z

Resolves #69 #92

The example is updated and routing/devtools are working as expected—routing remains synced with store and devtools, instrumentation toggles sync with routing (reset, revert, sweep), etc

A couple of things to consider:

Should the state keys be updated match react-router (ie, pathname instead of path)
Should we include including query support now that it's part of the LocationDescriptorObject? i know it's a common feature request and would be trivial to implement

Based on the new API, I'm thinking we should just mirror the whole history.LocationDescriptorObject—it's very simple and contains all relevant state in window.location...

state.routing = { pathname, search, query, state }; // mirror history.LocationDescriptorObject

@jlongster @kjbekkelund ellbee what do you guys think?

kimjoar · 2015-12-07T09:35:11Z

src/index.js

+
+      const method = routing.replace ? 'replace' : 'push';
+      const locationParam = (!!routing.state && typeof routing.path === 'String') ? 
+        routing.path : { state: routing.state, pathname: routing.path}


Is there a reason for switching between string and object based on state? Much easier to read the code if we can always send an object.

I agree we can just send the object, it works both ways will rebase

kimjoar · 2015-12-07T09:44:31Z

I like the idea of renaming path to pathname to be in sync with history

kimjoar · 2015-12-07T09:47:45Z

src/index.js

@@ -92,7 +90,7 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {

  const unsubscribeHistory = history.listen(location => {
    const route = {
-      path: history.createPath(location),
+      path: history.createHref(location),


Why is this needed?

it is needed to extract the url encoded path for current implementation. if we decided to mirror history location objects with query params, this line will not be necessary

This is the line that is breaking the tests in the browser. Changing back to createPath makes the tests pass.

i tried createPath but devtools tests were still failing... did you update deps?

Whoops, missed something... See new comment.

ellbee · 2015-12-07T10:50:49Z

Do we want to be using beta releases in the examples? We should bear in mind that people are going to be treating this code as best practice. I will add that I've been using the Devtools beta release and think that it is solid. Maybe the decision to use a beta should be on a case by case basis.

This PR also bumps webpack to the 2.0 beta release, can we justify this? It gives off some peer dependency warnings on npm install

kimjoar · 2015-12-07T11:10:01Z

@ellbee I agree — I thought about raising the same issue. I'm generally not a big fan of relying on beta releases in code people will copy when learning something new. I would prefer this PR without the beta releases, especially the Webpack upgrade.

justingreenberg · 2015-12-07T11:10:04Z

@ellbee i agree that redux-devtools@3beta is stable... in terms of webpack, i don't know too much about v2 but it's being used in production and has a solid team behind it. there's virtually no changes to the webpack api itself, so very easy to revert—totally up to you guys

ellbee · 2015-12-07T11:45:04Z

test/createTests.js

@@ -160,14 +159,15 @@ module.exports = function createTests(createHistory, name, reset = defaultReset)

      beforeEach(() => {
        history = createHistory();
-        const finalCreateStore = compose(devTools())(createStore);
+
+        const finalCreateStore = compose(instrument())(createStore);
        store = finalCreateStore(combineReducers({
          routing: routeReducer
        }));
        devToolsStore = store.devToolsStore;


Should be store.liftedStore

gaboesquivel · 2015-12-07T17:36:48Z

👍

justingreenberg · 2015-12-08T07:23:18Z

routing.path → routing.pathname to match history.location object keys

@jlongster @ellbee if we're syncing pathname and state, why not sync the full LocationDescriptor?

type LocationDescriptorObject = {
  pathname: Pathname;
  search: Search;
  query: Query;
  state: LocationState;
};

type LocationDescriptor = LocationDescriptorObject | Path;

it's super minimal, and there's really nothing in there that would pollute the redux store. query support could be optional but supported since CreateHistoryEnhancer has to be used to instantiate history:

import { createHistory, useQueries } from 'history'
const history = useQueries(createHistory)()

history.useQueries parses and passes query strings to history.location (LocationDescriptor above) and stringifies outgoing queries when creating or pushing paths with query key:

history.createPath({ pathname: '/the/path', query: { the: 'query' } })
history.push({ pathname: '/the/path', query: { the: 'query' } })

please let me know what you guys think

kimjoar · 2015-12-08T07:43:13Z

It definitely seems easier to support query params with the new release of history. If we do that change we should probably change pushPath and replacePath to parameters on the same format as history.push/replace, otherwise we need to keep adding stuff to the api.

It ends up being a huge change. I would rather see a clean PR with push+replace, plus the needed updates to get that working, but without all the other changes, then new PRs with the other stuff (upgrade to betas of devtools and webpack, change to pathname, support for queries, etc). That makes it far easier to be sure that we've thought through everything and added the necessary new tests etc.

ellbee · 2015-12-08T07:56:33Z

I agree with @kjbekkelund on both points.

justingreenberg · 2015-12-08T08:53:20Z

ok cool i moved README updates to #93, removed webpack beta and reverted path update

kimjoar · 2015-12-08T09:07:17Z

test/browser/index.js

@@ -2,4 +2,4 @@ const { createHashHistory, createHistory } = require('history');
 const createTests = require('../createTests.js');

 createTests(createHashHistory, 'Hash History', () => window.location = '#/');
-createTests(createHistory, 'Browser History', () => window.history.replaceState(null, null, '/'));


Any reason this is needed?

kimjoar · 2015-12-08T09:08:22Z

Nice, I'll test it out later today :)

New Devtools and Webpack releases are imminent. I’m not sure why devtools tests are failing on this commit I will look into more tomorrow. If you run the example, everything works beautifully.

jlongster · 2015-12-10T04:18:36Z

I don't have time to go through all of this right now, but I'm a little worried because right now we don't tie ourselves to any version of history. I thought the pushState and such methods were a stable API; now that those are changing we need to actually use peer dependencies or something. We can't just assume any version of redux-simple-router will work with any version of history.

Which kind of sucks. It gets tricky to make sure the user is using the right version of history with the right version of this lib. We need to look into how to make that work.

justingreenberg · 2015-12-10T06:24:48Z

@jlongster no worries! this pr just brings things up to date with [email protected] and the new history method names (stops the constant warnings for createHref, pushState, replaceState, etc.)

Now that we’ve hit 1.0, we are dedicated to smooth upgrade paths — @ryanflorence

since rackt is implementing more formal versioning it probably makes sense for redux-simple-router to track it's siblings ([email protected] is around the corner)

in terms of adding new features, ie. support for query and search, @kjbekkelund started a conversation and outlined some implementation options in #95

kimjoar · 2015-12-10T15:24:17Z

@justingreenberg As we need to think more about how to handle the history upgrade, maybe you could split this into two PRs? One for devtools and one for history, then we could look at getting the devtools stuff in now at least.

justingreenberg · 2015-12-14T06:45:59Z

@kjbekkelund now that devtools is stable i'll submit new PR in the morning for devtools update

@jlongster what are you thinking in terms of other updates to sync up with new history api? since there are merge conflicts and i'm splitting out devtools anyway, would you like me to to close this PR, clean them up and submit new pr? please let me know :)

kimjoar · 2015-12-14T07:10:47Z

@justingreenberg Awesome 💯

jlongster · 2015-12-18T21:02:38Z

@justingreenberg You can create a new clean PR with just updating the history integration. Just make sure to link this one so it's easy to follow the discussion.

I have more time now so when you open a new PR I'll take a closer look at it.

jlongster · 2015-12-18T21:27:07Z

Also I agree that we should update to whatever the newest history API is internally and we can open a separate issue for possible changes to our API to better support location objects if we want to do that.

Deprecate history.replaceState and history.pushState

e1710be

kimjoar reviewed Dec 7, 2015
View reviewed changes

ellbee reviewed Dec 7, 2015
View reviewed changes

kimjoar mentioned this pull request Dec 7, 2015

use history.push instead of history.pushState #85

Closed

kimjoar mentioned this pull request Dec 8, 2015

history.js deprecated pushState in favor of push #92

Closed

justingreenberg mentioned this pull request Dec 8, 2015

Update README and add CHANGELOG #93

Merged

justingreenberg mentioned this pull request Dec 8, 2015

Add a CHANGELOG and update the README #90

Closed

kimjoar reviewed Dec 8, 2015
View reviewed changes

justingreenberg changed the title ~~Update history and devtools~~ Update History and DevTools Dec 8, 2015

Update Babel, DevTools and WebPack

0b386e8

New Devtools and Webpack releases are imminent. I’m not sure why devtools tests are failing on this commit I will look into more tomorrow. If you run the example, everything works beautifully.

justingreenberg mentioned this pull request Dec 19, 2015

chore(history): update to history ^0.14.0 internally #131

Merged

justingreenberg closed this Dec 19, 2015

justingreenberg deleted the update-history branch December 19, 2015 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update History and DevTools #89

Update History and DevTools #89

justingreenberg commented Dec 7, 2015

kimjoar Dec 7, 2015

justingreenberg Dec 7, 2015

kimjoar commented Dec 7, 2015

kimjoar Dec 7, 2015

justingreenberg Dec 7, 2015

ellbee Dec 7, 2015

justingreenberg Dec 7, 2015

ellbee Dec 7, 2015

ellbee commented Dec 7, 2015

kimjoar commented Dec 7, 2015

justingreenberg commented Dec 7, 2015

ellbee Dec 7, 2015

kimjoar Dec 7, 2015

gaboesquivel commented Dec 7, 2015

justingreenberg commented Dec 8, 2015

kimjoar commented Dec 8, 2015

ellbee commented Dec 8, 2015

justingreenberg commented Dec 8, 2015

kimjoar Dec 8, 2015

kimjoar commented Dec 8, 2015

jlongster commented Dec 10, 2015

justingreenberg commented Dec 10, 2015

kimjoar commented Dec 10, 2015

justingreenberg commented Dec 14, 2015

kimjoar commented Dec 14, 2015

jlongster commented Dec 18, 2015

jlongster commented Dec 18, 2015

Update History and DevTools #89

Update History and DevTools #89

Conversation

justingreenberg commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar commented Dec 7, 2015

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

ellbee commented Dec 7, 2015

kimjoar commented Dec 7, 2015

justingreenberg commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaboesquivel commented Dec 7, 2015

justingreenberg commented Dec 8, 2015

kimjoar commented Dec 8, 2015

ellbee commented Dec 8, 2015

justingreenberg commented Dec 8, 2015

Choose a reason for hiding this comment

kimjoar commented Dec 8, 2015

jlongster commented Dec 10, 2015

justingreenberg commented Dec 10, 2015

kimjoar commented Dec 10, 2015

justingreenberg commented Dec 14, 2015

kimjoar commented Dec 14, 2015

jlongster commented Dec 18, 2015

jlongster commented Dec 18, 2015