[added] Peer dependency on history package #2258

mjackson · 2015-10-13T17:57:39Z

Since we don't plan on making any breaking changes in the history package without a major version bump, we should be able to just have a peer dep on history ~1. Please review and merge if it looks good to you, @taion.

Fixes #2211
Fixes #2252

knowbody · 2015-10-13T18:08:28Z

@mjackson some tests are failing

mjackson · 2015-10-13T18:11:04Z

ya, i'll look into it @knowbody. Maybe history 1.12.5 broke something...

taion · 2015-10-13T18:11:30Z

Just need to also add it to devDependencies actually.

Should change README from

$ npm install [email protected]

to

$ npm install history react-router@latest

ETA: Plus maybe add a line or two of copy explaining why the history package is needed.

knowbody · 2015-10-13T18:12:13Z

I think the @latest points to 0.13.4 @taion

mjackson · 2015-10-13T18:13:20Z

No, @latest points to 1.0.0-rc3:

$ npm dist-tag ls
latest: 1.0.0-rc3

It did for a while, @knowbody, but I fixed it.

I'll make that change, @taion.

knowbody · 2015-10-13T18:14:29Z

oh, cool! what was the fix in the end @mjackson? I thought that was something to do with how npm deals with it

taion · 2015-10-13T18:16:25Z

Yeah it's a weird NPM quirk. Despite 1.0.0-rc3 being tagged as latest, npm i react-router will give you v0.13.4, but npm i react-router@latest will give you v1.0.0-rc3.

Fixes #2211 Fixes #2252

mjackson · 2015-10-13T18:22:37Z

Bah, still failing. I'll have to look into it later.

timdorr · 2015-10-13T18:31:16Z

Ah, it's because of this. history fixed it's hash handling, so you should just need to update those tests and it should pass.

taion · 2015-10-13T18:53:30Z

👍 I think this should clarify things a lot.

Maybe I'd write the copy as something like

React Router builds on top of History to handle navigation and the history stack, so you'll need to install both libraries.

timdorr · 2015-10-13T21:15:42Z

@mjackson That commit should fix it. We should probably add a npm install command to the test script so that we're sure we're building against the right deps.

mjackson · 2015-10-13T22:22:33Z

Thanks, @timdorr! :D

We should probably add a npm install command to the test script so that we're sure we're building against the right deps.

I don't understand that part. Travis already does an npm install before it runs the tests.

[added] Peer dependency on history package

knowbody · 2015-10-13T22:25:34Z

React Router builds on top of History to handle navigation and the history stack, so you'll need to install both libraries.

@taion would be great if you add this line to the docs :) (maybe just add the line that there is no need for it if someone is using hash history)

SpainTrain · 2015-10-13T23:04:09Z

we should be able to just have a peer dep on history ~1

Shouldn't the peer dep track the minor version of history from dev deps? Since react-router directly imports and uses history [1,2,3], if a user has (for example) installed 1.11 and react-router uses a feature from 1.12, they are going to have a bad time.

[1] https://github.com/rackt/react-router/blob/master/modules/useRoutes.js#L2
[2] https://github.com/rackt/react-router/blob/master/modules/Router.js#L3
[3] https://github.com/rackt/react-router/blob/master/modules/match.js#L2

taion · 2015-10-13T23:39:23Z

It shouldn't necessarily track what's in devDependencies because the tests could be using features that the core code does not. We could specify a caret range if we need to - do we, though?

SpainTrain · 2015-10-13T23:47:52Z

FWIW, tested this locally and plays out in practice:

with [email protected] and react-router@master (built with npm run build)

TypeError: history.createLocation is not a function
    at match (/home/spainhower/repos/homebuddy-webapp/node_modules/react-router/lib/match.js:55:56)
    at getRenderProps (/home/spainhower/repos/homebuddy-webapp/dist/webpack:/app/server/react-app-handler.js:36:12)
    at /home/spainhower/repos/homebuddy-webapp/dist/webpack:/app/server/react-app-handler.js:57:26
    at Layer.handle [as handle_request] (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/layer.js:95:5)
    at next (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/route.js:131:13)
    at Route.dispatch (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/layer.js:95:5)
    at /home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:277:22
    at param (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:349:14)
    at param (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:365:14)
    at Function.process_params (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:410:3)
    at next (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:271:10)
    at Function.handle (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:176:3)
    at router (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:46:12)

with [email protected] and react-router@master (built with npm run build):

I can provide the full source of react-app-handler if it's useful, but those lines in the stack are just taken straight from the server rendering docs (but promisified):

const matchAsPromised = Promise.promisify(match);
....
    let location = req.url;

    return matchAsPromised({routes, location})
        .spread((redirectLocation, renderProps) => {
....

mjackson · 2015-10-14T05:21:32Z

Ya, we should prob include minor version as well for new features that we
may rely on that are introduced in minor versions. Happy to accept a PR :)

Thanks for bringing it up
On Tue, Oct 13, 2015 at 4:48 PM Mike S [email protected] wrote:

FWIW, tested this locally and plays out in practice:

with [email protected] and react-router@master (built with npm run build)

TypeError: history.createLocation is not a function
at match (/home/spainhower/repos/homebuddy-webapp/node_modules/react-router/lib/match.js:55:56)
at getRenderProps (/home/spainhower/repos/homebuddy-webapp/dist/webpack:/app/server/react-app-handler.js:36:12)
at /home/spainhower/repos/homebuddy-webapp/dist/webpack:/app/server/react-app-handler.js:57:26
at Layer.handle as handle_request
at next (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/route.js:131:13)
at Route.dispatch (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/route.js:112:3)
at Layer.handle as handle_request
at /home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:277:22
at param (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:349:14)
at param (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:365:14)
at Function.process_params (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:410:3)
at next (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:271:10)
at Function.handle (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:176:3)
at router (/home/spainhower/repos/homebuddy-webapp/node_modules/express/lib/router/index.js:46:12)

with [email protected] and react-router@master (built with npm run build):

https://assets-git-camo.f3mw1.com/2a5d1b38e92af8174c29da77216e984dc433d81b/687474703a2f2f7777772e646f6d7573696e632e636f6d2f626c6f672f77702d636f6e74656e742f75706c6f6164732f323031322f30392f626c6f672e6a7067

I can provide the full source of react-app-handler if it's useful, but
those lines in the stack are just taken straight from the server rendering
docs (but promisified):

const matchAsPromised = Promise.promisify(match);
....
let location = req.url;
return matchAsPromised({routes, location})
    .spread((redirectLocation, renderProps) => {
....

—
Reply to this email directly or view it on GitHub
#2258 (comment).

* Set `history` peer dep to minor version needed by react-router core See also: remix-run#2258 (comment)

SpainTrain · 2015-10-14T05:26:46Z

Happy to accept a PR

magic words 🐰 #2268

[added] Peer dependency on history package

3cc7d6d

Fixes #2211 Fixes #2252

mjackson force-pushed the history-peer-dep branch from 121ff94 to 3cc7d6d Compare October 13, 2015 18:18

Fix for pushState change in history useQueries.

8f72913

mjackson added a commit that referenced this pull request Oct 13, 2015

Merge pull request #2258 from rackt/history-peer-dep

fc2584c

[added] Peer dependency on history package

mjackson merged commit fc2584c into master Oct 13, 2015

gaearon deleted the history-peer-dep branch October 13, 2015 22:26

SpainTrain pushed a commit to SpainTrain/react-router that referenced this pull request Oct 14, 2015

Update history peer dep to ^1.12.0

52a66ce

* Set `history` peer dep to minor version needed by react-router core See also: remix-run#2258 (comment)

SpainTrain mentioned this pull request Oct 14, 2015

Update history peer dep to ^1.12.0 #2268

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Uh oh!

[added] Peer dependency on history package #2258

[added] Peer dependency on history package #2258

Uh oh!

Conversation

mjackson commented Oct 13, 2015

Uh oh!

knowbody commented Oct 13, 2015

Uh oh!

mjackson commented Oct 13, 2015

Uh oh!

taion commented Oct 13, 2015

Uh oh!

knowbody commented Oct 13, 2015

Uh oh!

mjackson commented Oct 13, 2015

Uh oh!

knowbody commented Oct 13, 2015

Uh oh!

taion commented Oct 13, 2015

Uh oh!

mjackson commented Oct 13, 2015

Uh oh!

timdorr commented Oct 13, 2015

Uh oh!

taion commented Oct 13, 2015

Uh oh!

timdorr commented Oct 13, 2015

Uh oh!

mjackson commented Oct 13, 2015

Uh oh!

knowbody commented Oct 13, 2015

Uh oh!

SpainTrain commented Oct 13, 2015

Uh oh!

taion commented Oct 13, 2015

Uh oh!

SpainTrain commented Oct 13, 2015

Uh oh!

mjackson commented Oct 14, 2015

Uh oh!

SpainTrain commented Oct 14, 2015

Uh oh!

Uh oh!