Skip to content

Update examples and documentations for upstream-driven API changes #2669

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
littlee opened this issue Dec 8, 2015 · 38 comments
Closed

Update examples and documentations for upstream-driven API changes #2669

littlee opened this issue Dec 8, 2015 · 38 comments

Comments

@littlee
Copy link

littlee commented Dec 8, 2015

I am interested in the huge-app example, but when I run it on my PC,
it shows many errors, how do I fix it?
qq 20151208114028

@taion
Copy link
Contributor

taion commented Dec 8, 2015

We haven't had a chance to update the examples yet - we will do so soon.

@taion taion changed the title Errors in example Update examples and documentation for new <Link> API Dec 8, 2015
@taion taion added the docs label Dec 8, 2015
@taion taion changed the title Update examples and documentation for new <Link> API Update examples and documentations for upstream-driven API changes Dec 8, 2015
@taion taion added this to the v1.1 milestone Dec 8, 2015
@timdorr timdorr modified the milestone: v1.1 Dec 8, 2015
@littlee
Copy link
Author

littlee commented Dec 9, 2015

(I am a little confused with the chang log...)
Which version of history should I install?
npm install history or npm install [email protected]?

@taion
Copy link
Contributor

taion commented Dec 9, 2015

Use 1.13.x for now.

@littlee
Copy link
Author

littlee commented Dec 9, 2015

👌

@ryanflorence
Copy link
Member

the master branch has new API that our own internals aren't using yet, so the master branch spits out the warnings until we can finish working on all of it.

git checkout v1.0.2 and you won't have the warnings. npm install react-router will likewise not have the warnings.

@ryanflorence ryanflorence removed this from the v1.1 milestone Dec 10, 2015
@thenickcox
Copy link

I can confirm I don't see this anymore after upgrading to history 1.13.1 and react-router 1.0.2.

@mik01aj
Copy link
Contributor

mik01aj commented Dec 14, 2015

@thenickcox, thanks. I used your version numbers and it helped :)

@uinz
Copy link

uinz commented Dec 16, 2015

    "babel-loader": "^6.2.0",
    "history": "^1.16.0",
    "isomorphic-fetch": "^2.2.0",
    "koa": "^1.1.2",
    "react": "^0.14.3",
    "react-dom": "^0.14.3",
    "react-redux": "^4.0.1",
    "react-router": "^1.0.2",
    "redux": "^3.0.5",
    "redux-logger": "^2.2.1",
    "redux-thunk": "^1.0.2",
    "webpack": "^1.12.9"

I use [email protected] and [email protected], but it still warning me.
image

when I down to 1.1.3 it is fixed.

@thenickcox thanks.

@taion
Copy link
Contributor

taion commented Dec 16, 2015

And that's why the peer dependency says 1.13.x!

@ryanflorence
Copy link
Member

Use the one npm tells you to.
On Wed, Dec 16, 2015 at 7:06 AM yinz [email protected] wrote:

"babel-loader": "^6.2.0",
"history": "^1.16.0",
"isomorphic-fetch": "^2.2.0",
"koa": "^1.1.2",
"react": "^0.14.3",
"react-dom": "^0.14.3",
"react-redux": "^4.0.1",
"react-router": "^1.0.2",
"redux": "^3.0.5",
"redux-logger": "^2.2.1",
"redux-thunk": "^1.0.2",
"webpack": "^1.12.9"

I use [email protected] and [email protected], but it still warning me.
[image: image]
https://cloud.githubusercontent.com/assets/12208108/11844385/81f082f4-a449-11e5-90ea-b372e82ee451.png


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

@ajsharp
Copy link

ajsharp commented Dec 19, 2015

Shouldn't this be packaged as a normal dependency, not a peer?

@taion
Copy link
Contributor

taion commented Dec 19, 2015

Not for the current version. For the next one, yes.

@sinaler
Copy link

sinaler commented Dec 22, 2015

My console is still full of warnings

@timdorr
Copy link
Member

timdorr commented Dec 22, 2015

@sercaninaler Make sure you have [email protected] installed.

@sinaler
Copy link

sinaler commented Dec 22, 2015

These are installed:
"history": "^1.13.1",
"react": "0.14.3",
"react-dom": "0.14.3",
"react-router": "1.0.2",

But still getting error on;
[email protected]:122

Is it because I have Babel 6 as compiler?

@timdorr
Copy link
Member

timdorr commented Dec 22, 2015

You have a caret range on history. It must be exactly 1.13.1. Do not install history 1.14.0 or higher right now.

@ajsharp
Copy link

ajsharp commented Dec 22, 2015

I'd like to add a vote of support for a bugfix release to the 0.13.x release line to add the correct version of history to dependencies. If the library requires it to function correctly, it should be a hard dependency, not a peer.

@timdorr
Copy link
Member

timdorr commented Dec 22, 2015

0.13.x doesn't use history.

@ajsharp
Copy link

ajsharp commented Dec 22, 2015

Sorry, I confused the version numbers -- I meant the 1.0.2 release line: https://github.com/rackt/react-router/blob/2153e2cf0d691c141257eaf29cf230b0bc0f789f/package.json

@timdorr
Copy link
Member

timdorr commented Dec 22, 2015

It's fixed in the 1.0.x branch. @ryanflorence needs to package it up and release as 1.0.3.

@sinaler
Copy link

sinaler commented Dec 22, 2015

Now It's ok, but we wait for the release, but thanks so much for the great effort.

@ajsharp
Copy link

ajsharp commented Dec 22, 2015

So the bug in history was fixed in 1.17.0? Would you mind explaining why it's still listed as a peer dependency and not a normal dependency? My understanding of peer dependencies is that they're meant for "plugins", but history doesn't seem to meet that description, as it's required for react-router to function.

@timdorr
Copy link
Member

timdorr commented Dec 22, 2015

Until 2.0.0, we never directly used history (outside of tests). As such, it better fit as a peer dep so that libraries implementing react-router and history (such as redux-router, react-router-relay, etc) would have better control over its versioning. See #2211 for more detail.

Now that we're going to start exporting our own history singletons in 2.0.0, it's been moved back to a regular dependency.

Unfortunately, NPM 3 has made the peerDep experience rocky. We would have kept it as a peerDep on 2.0.0 if we didn't go the singleton route (pun intended), but I know I probably would have worked on some NPM PRs to improve that UX quite a bit.

@ajsharp
Copy link

ajsharp commented Dec 22, 2015

Thanks for the explanation, much appreciated. I might also suggest updating the README with the specific version of history that needs to be installed. Currently it just says npm install history

@ajsharp
Copy link

ajsharp commented Dec 22, 2015

Until 2.0.0, we never directly used history (outside of tests).

In #2697 the author of that issue points to two points in the 1.0.x release branch where react-router does use history directly. I understand that it's been moved to a normal dependency in the 2.0.x branch, but it still seems clear that it should be a normal dependency in the 1.0.x branch, because it's already proven to create errors for users trying to install this library per the instructions laid out in the README.

If I can install react-router and get errors b/c I haven't installed the correct minor version of history, that's a big problem. I understand that this problem is partly due to the fact that history possibly shipped a minor release that broke existing functionality (e.g. broke semver) within react-router.

In that case, the README of this repo should be updated to communicate loud and clear VERSION X of history breaks react-router, only use VERSION Y. The fact that many people will have to find this issue to discover the solution to that seemingly common scenario really sucks.

@taion
Copy link
Contributor

taion commented Dec 22, 2015

The peer dep issue is getting fixed for now as soon as we cut 1.0.3. There's not really much more to discuss here - the way we import hash history in v1.0.x meant that the peer dependency made the most sense.

@stephensong
Copy link

From my package.json

"history": "^1.17.0",
...
"react-router": "^1.0.3",

I am still getting a sh*tload of warnings. What is the correct setup? Is there such a thing?

I'm beginning to think my nightmare is never going to end. Constantly trying to upgrade innumerable interdependant components in search of a stable, working system. This most recent search started with Babel 6 (oh the horror) and has been going now for nigh on two months.

@taion
Copy link
Contributor

taion commented Jan 13, 2016

That is the correct setup on 1.0.x. Make sure you have the actual correct package versions installed.

@stephensong
Copy link

Yep, Those are the installed versions alright.

Looking at the source of lib/Link.js, on line 122 I see:

props.href = history.createHref(to, query);

which is causing some of the warnings. Is that not right?

@timdorr
Copy link
Member

timdorr commented Jan 13, 2016

@stephensong Welcome to the Node ecosystem! 😉

Easiest way to check that you've got the right versions installed: npm list react-router history

If you don't see the right ones, run npm upgrade react-router history and you should end up on 1.0.3 and 1.17.0 respectively.

@stephensong
Copy link

As I said previously, those are the installed versions.

gary@ubuntu-box:~/dev/psq-spa$ npm list react-router history
[email protected] /home/gary/dev/psq-spa
├── [email protected] 
└── [email protected] 

Honestly ...

@timdorr
Copy link
Member

timdorr commented Jan 13, 2016

You had listed what was in package.json, but that's not a hard requirement (npm/node doesn't behave like Bundler in Rubyland and load dependencies based on a config file).

What are one of the warnings you are getting? We don't normally emit warnings with those versions (Note: the location one is expected)

@stephensong
Copy link

Warning: the query argument to createHref is deprecated; use a location descriptor instead

Surely you jest. The system issues warnings about it's own code in the 'fixed' version. I can see why you can say it is expected? Not just expected, but inevitable, yes? Why not just fix it?

I can't speak for anybody else, of course, but it most certainly not expected by me.

Is this one also to be expected?

Warning: You cannot change <Router routes>; it will be ignored

@taion
Copy link
Contributor

taion commented Jan 13, 2016

The first warning is explicitly commented out in v1.17.0, which is why we asked if you had the correct version installed: https://github.com/rackt/history/blob/v1.17.0/modules/useQueries.js#L118

The latter one is indicative of your trying to do something that won't work.

@stephensong
Copy link

ok. my humblest apologies. I had some sort of unwanted caching effect going on with my (newly updated of course) hmr stuff, which somehow meant that what was being executed was not actually what was on the disk.

Fwiw (not very much I admit) in my defence, I was objecting to this line from Tim:

(Note: the location one is expected)

which, as it turns out, is not actually true is it.

Anyways, many thanks for your assistance in resolving my issue. Now on to the next problem on my list. Some day (hopefully in this calendar year) I will be able to actually get some more work done. I have to believe it to be so. ; - )

@timdorr
Copy link
Member

timdorr commented Jan 13, 2016

In the context of our test suite, yes, the 'Warning: Location "/" did not match any routes' warning is expected for that particular unit test on the 1.0.x branch.

@stephensong
Copy link

Ok, sorry again, my mistake. When you said "the location one" I incorrectly thought you were referring to the "use a location descriptor instead" bit.

Regarding, this one:

Warning: You cannot change <Router routes>; it will be ignored

looking at my code it seems to arise from the fact that I have wrapped my Router in a Root element, and then wrapped that with a call to redux connect. (see code at bottom). I don't think I am actually changing the routes, but the test

this.props.children === nextProps.children

fails somehow. Is this the wrong way to integrate redux and react-router, and, if so, what is the right way?

many tias, gary

function renderRoutes (history) {
    return (
    <Router key='router' history={history}>
      <Route component={App}>
        <Route path="/help" component={Help}/>
        ...
    </Router>
  );
}

function getRootChildren (props) {
  const rootChildren = [
      renderRoutes(props.history)
  ];
  return rootChildren;
}

const store = finalCreateStore(reducer, initialState);

class _root extends React.Component {
  render () {
    return (
      <div>{getRootChildren(this.props)}</div>
    )
  };
}
_root.propTypes = { history: PropTypes.object.isRequired  };

let Root = connect(state => state)(_root);

ReactDOM.render(
  <Provider store={store}>
    <Root history={history} />
  </Provider>
, document.getElementById('page-body'))


@timdorr
Copy link
Member

timdorr commented Jan 13, 2016

I would get rid of the Root component that you have there. It doesn't serve any purpose. I would put the Router directly in the Provider and make renderRoutes just return your route config, rather than the router instance itself.

This is probably better served by Stack Overflow, to be honest. Only @taion and I really have eyes on this issue. You'll get a lot more eyeballs on SO and people there are incentivized by karma points and such.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants