Skip to content
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

update missing deps (standalone examples) #71

Closed
wants to merge 1 commit into from

Conversation

asfktz
Copy link

@asfktz asfktz commented Sep 21, 2015

currently missing dependencies taken from the main repository.

this PR fix this by explicitly specify deps in examples/package.json

@asfktz asfktz changed the title update missing deps update missing deps in examples Sep 21, 2015
@asfktz asfktz changed the title update missing deps in examples update missing deps (standalone examples) Sep 21, 2015
"dependencies": {
"history": "^1.9.1",
"react": "^0.14.0-rc1",
"react-dom": "^0.14.0-rc1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really only work with React 0.14?

@doobleweb
Copy link

@AsaAyers it works if you wrap <ReduxRouter /> with a function
the original code used 0.14, I just followed it

about 'redux' yes, it depend on it right here:
https://github.com/rackt/redux-router/blob/master/examples/basic/index.js#L3

about peerDependency, I'm unfamiliar with it.
it it the case here?

@AsaAyers
Copy link

redux is also required in src/client.js. If redux-router were going to state that it depends on redux it could be a dependency or peerDependency. In this scenario lets say my project depends on redux@2 and redux-router depends on redux@3

If it is a dependency npm will happily install redux@2.x.x for my code to use and redux@3.x.x would be provided for redux-router to use. I don't know what problems this would cause, but I suspect it would produce errors and I'm sure it's not what is intended. Any errors here would happen at runtime.

If instead, it were a peerDependency then npm can complain that it can't find a single version of redux that satisfies both dependencies. This avoids the runtime error and instead causes the failure to show up during npm install

The 3rd option is to just not specify that dependency. It currently works because my project's dependencies will install a version of redux and redux-router will just use whatever it gets from require('redux'). The biggest problem here is that it would be possible to install a redux that isn't compatible with the router and you won't get an error about it until runtime.

@asfktz
Copy link
Author

asfktz commented Sep 22, 2015

I see.
so, if I understand it right,
redux-router should have a peerDependency of redux
while the examples should have it has dependency

@AsaAyers
Copy link

I'd leave that up to the owners here to decide, but that's the option I think I'd chose. I haven't upgraded to npm@3 yet, so this is based on my experience with npm@2 and what I have read about the upcoming changes.

Either way, I think it's outside the scope of this PR, which just updates the example app

@Scarysize
Copy link
Contributor

We experimented with peerDependency, this is still causing trouble with some npm versions. I think the current state of dependencies is looks fine and it doesn´t seem to cause any major issues.

@Scarysize Scarysize closed this Jan 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants