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

Fix dependencies of history #180

Closed
wants to merge 1 commit into from

Conversation

koba04
Copy link

@koba04 koba04 commented Nov 18, 2015

I tried npm shrinkwrap on an app that has redux-router as dependencies.
But an error occurs because history exists in devDependencies instead of dependencies.

➜  npm i -S [email protected]
[email protected] node_modules/redux-router
└── [email protected]
➜  npm shrinkwrap
npm ERR! Darwin 15.0.0
npm ERR! argv "/Users/koba04/.anyenv/envs/ndenv/versions/v4.2.2/bin/node" "/Users/koba04/.anyenv/envs/ndenv/versions/v4.2.2/bin/npm" "shrinkwrap"
npm ERR! node v4.2.2
npm ERR! npm  v2.14.7

npm ERR! Problems were encountered
npm ERR! Please correct and try again.
npm ERR! extraneous: [email protected] /Users/koba04/Desktop/test/node_modules/redux-router/node_modules/history
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/koba04/Desktop/test/npm-debug.log
➜  ls node_modules
redux-router

This PR is to fix that issue.
Thanks.

@Scarysize
Copy link
Contributor

@koba04 Hey, I think adding historyas a peerDependency would make more sense. react-router also handles it this way. What do think, might this be a problem for shrinkwrap?

@koba04
Copy link
Author

koba04 commented Nov 24, 2015

@Scarysize
Thank you! That's make sense.
In npm v2, peerDependencies works. But it doesn't work in npm v3.

➜  npm i -S history @koba04/[email protected]

> [email protected] postinstall /Users/koba04/Desktop/test/node_modules/history
> node ./npm-scripts/postinstall.js

[email protected] /Users/koba04/Desktop/test
├─┬ @koba04/[email protected]
│ └── [email protected]
└─┬ [email protected]
  ├─┬ [email protected]
  │ └─┬ [email protected]
  │   └── [email protected]
  ├── [email protected]
  └── [email protected]

npm WARN EPACKAGEJSON [email protected] No description
npm WARN EPACKAGEJSON [email protected] No repository field.
➜  ls node_modules
@koba04      deep-equal   history      invariant    js-tokens    loose-envify qs           warning
➜  ls node_modules/@koba04/redux-router/node_modules
history
➜  npm shrinkwrap
npm ERR! Darwin 15.0.0
npm ERR! argv "/Users/koba04/.anyenv/envs/ndenv/versions/v4.2.2/bin/node" "/Users/koba04/.anyenv/envs/ndenv/versions/v4.2.2/bin/npm" "shrinkwrap"
npm ERR! node v4.2.2
npm ERR! npm  v3.4.1

npm ERR! Problems were encountered
npm ERR! Please correct and try again.
npm ERR! extraneous: [email protected] /Users/koba04/Desktop/test/node_modules/@koba04/redux-router/node_modules/history
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/koba04/Desktop/test/npm-debug.log

(@koba04/[email protected] is peerDependencies version)

I suppose it's a problem to be installed history as redux-router's dependencies.
But I'm not sure why history is installed as redux-router's dependencies regardless of whether it is specified dependencies or not...

react, redux, react-router and others are not installed as dependencies...

@Scarysize
Copy link
Contributor

Right now npm install redux-router@latestshouldn´t install anything except redux-router. I will do some testing today and determine how to add the dependency.

@koba04
Copy link
Author

koba04 commented Dec 2, 2015

@Scarysize

Thanks for adding history to dependencies.
But, npm-shrinkwrap is still broken.
Because a published redux-router tarball is including node_modules/history.
I think a package tarball should not include node_modules/.

➜  tree -L 2 ~/Downloads/package/
/Users/koba04/Downloads/package/
├── LICENSE
├── README.md
├── lib
│   ├── ReduxRouter.js
│   ├── __tests__
│   ├── actionCreators.js
│   ├── client.js
│   ├── constants.js
│   ├── historyMiddleware.js
│   ├── index.js
│   ├── isActive.js
│   ├── matchMiddleware.js
│   ├── reduxReactRouter.js
│   ├── replaceRoutesMiddleware.js
│   ├── routeReplacement.js
│   ├── routerStateEquals.js
│   ├── routerStateReducer.js
│   ├── server.js
│   ├── serverModule.js
│   └── useDefaults.js
├── node_modules
│   └── history
├── package.json
└── server.js

Would you please make sure of a way to publish this package?
Thanks.

@martinandert
Copy link

The following packages should be in the dependencies section of package.json (not in devDependencies), since stuff is imported from these packages inside this project's src/*.js modules:

  • redux
  • react-redux
  • react-router
  • history
  • deep-equal

Maybe some of these should rather be peerDependencies.

@koba04
Copy link
Author

koba04 commented Dec 21, 2015

@Scarysize

Because a published redux-router tarball is including node_modules/history.

I found related issues.
npm/npm#9642
npm/fstream-npm#15

Can you republish this with npm@>2.14.10 || >3.4.0?

@Scarysize
Copy link
Contributor

@koba04 I think we managed to find the bug here #218
We are going to move history back to devDeps and republish with deleting node_modules/history first.

@koba04
Copy link
Author

koba04 commented Dec 21, 2015

@Scarysize Thanks!

@Scarysize Scarysize closed this Dec 24, 2015
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

Successfully merging this pull request may close these issues.

3 participants