acdlite / redux-router

Redux bindings for React Router – keep your router state inside your Redux store
MIT License
2.3k stars 216 forks source link

history dependency screwed up since beta5 #218

Closed texttechne closed 8 years ago

texttechne commented 8 years ago

As has been mentioned in two other issues (acdlite/redux-router/issues/193, acdlite/redux-router/issues/198), the history dependency is kind of screwed up.

Installing it only shows deep-equal as dependency, history is missing:

redux-router@1.0.0-beta5 node_modules\redux-router
└── deep-equal@1.0.1

Furthermore, inspecting package.json of history dependency shows ... hmm, well, it is not shrinkwrapped, but somehow cached... haven't seen it before (and who is franz?):

{
  "_args": [
    [
      "history@^1.12.0",
      "/Users/franz/Sites/redux-router"
    ]
  ],
  "_from": "history@>=1.12.0 <2.0.0",
  "_id": "history@1.13.1",
  "_inCache": true,
  "_installable": true,
  "_location": "/history",
  "_nodeVersion": "5.0.0",
  "_npmUser": {
    "email": "mjijackson@gmail.com",
    "name": "mjackson"
  },
  "_npmVersion": "3.3.6",
  "_phantomChildren": {},
  "_requested": {
    "name": "history",
    "raw": "history@^1.12.0",
    "rawSpec": "^1.12.0",
    "scope": null,
    "spec": ">=1.12.0 <2.0.0",
    "type": "range"
  },
  "_requiredBy": [
    "/"
  ],
  "_resolved": "https://registry.npmjs.org/history/-/history-1.13.1.tgz",
  "_shasum": "1d0664e667f031d5757ef73e81ef847beb3aedcd",
  "_shrinkwrap": null,
  "_spec": "history@^1.12.0",
  "_where": "/Users/franz/Sites/redux-router",

  ... fine from here on

As mentioned in the other issues, node_modules won't get installed properly.

My guess is, that this has been committed (it actually is possible, if you exclude your node_modules after having committed)...

Beta4 is not affected, but at that point history was a devDep.

Scarysize commented 8 years ago

Hey, this seems to be a problem on my side. Will look into it. @texttechne what would be your solution for the history dependency. ´devDependency´ seems to cause problems with shrink-wrap, normal dependency seems to have issues with older npm versions...

texttechne commented 8 years ago

Ok, digged a little deeper. The fault is to be found within npm. Relevant issues:

209 (and #54 which lead to changing history as dep instead of devDep)

react-router ran into the same issue. They changed their deploy script. Issue: rackt/react-router/issues/2195 Fix: rackt/react-router@3be6a2d2329edf47fe2911e6f384a50986dc1fe5

texttechne commented 8 years ago

@Scarysize In regard to devDep vs. dep: history is a peer dependency in disguise of a devDep (https://github.com/acdlite/redux-router/issues/54#issuecomment-140866571). So you should revert to declare it as devDep.

Then the question arises how you release redux-router. Manually? At this point a script like that of react-router would come in handy...

Scarysize commented 8 years ago

@texttechne Thanks for making the effort, I will look into react-routers solution. I pushed a branch earlier today with the history module moved back to devDependencies. When I found a solution to the publishing bug, I will merge it.

Scarysize commented 8 years ago

https://github.com/acdlite/redux-router/blob/release-script/release.sh

So i looked at react-routers release script and made simplified version. I think this should work (tested it without the npm publish). @texttechne Check it out :)

texttechne commented 8 years ago

@Scarysize right now, you have history referenced in devDeps and deps (master as well as branch release-script). It should only be referenced as devDep, since it technically is a peerDependency (see my last comment).

Otherwise I cannot test that with the release script branch: sources are missing, only package.json and node_modules when running npm install...

Scarysize commented 8 years ago

@texttechne So the release script is now obsolete. The npm bug has been fixed, the react-router guys brought this to my attention. Also we will drop history completely, as will react-router. The only occurrence of a direct import from history is in our sever module, which we will try to work around.

koba04 commented 8 years ago

@Scarysize https://registry.npmjs.org/redux-router/-/redux-router-1.0.0-beta6.tgz

redux-router-1.0.0-beta6 is still including node_modules/history in the tarball. This will be fixed in the next release?

mjrussell commented 8 years ago

@koba04 its actually a bug in NPM itself, not the in the packaging of the app. Upgrading node from v4.2.1 to v4.2.4 solved the problem for me. If you can't upgrade, you might add a small post install script to your package.json that deletes the unnecessary history. Until we have no dependency at all on history (which should be possible after upgrading to react-router 2.x), those are the only solutions.

Scarysize commented 8 years ago

@mjrussell we might want to add this to the README...depending on how far away the next release is.

koba04 commented 8 years ago

I know this is a npm's bug. However the bug is about relating the npm packaging process, right? I'm using npm v4.2.4, But an error occurs.

➜  npm i -S redux-router@1.0.0-beta6
npm WARN package.json test@1.0.0 No description
npm WARN package.json test@1.0.0 No repository field.
npm WARN package.json test@1.0.0 No README data
redux-router@1.0.0-beta6 node_modules/redux-router
└── deep-equal@1.0.1
➜  npm shrinkwrap
npm ERR! Darwin 15.2.0
npm ERR! argv "/Users/koba04/.anyenv/envs/ndenv/versions/4.2.4/bin/node" "/Users/koba04/.anyenv/envs/ndenv/versions/4.2.4/bin/npm" "shrinkwrap"
npm ERR! node v4.2.4
npm ERR! npm  v2.14.12

npm ERR! Problems were encountered
npm ERR! Please correct and try again.
npm ERR! missing: install@^0.4.1, required by test@1.0.0
npm ERR! missing: npm@^3.5.2, required by test@1.0.0
npm ERR! extraneous: history@1.17.0 /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

(A proposal to add custom post install script is fine for me. thanks.)

Scarysize commented 8 years ago

Hm I could try to remove my local history package and republish the module. Wouldn't be much work and I would be interested if this might help.

Scarysize commented 8 years ago

I just republished as 1.0.0-beta7 and tried a shrink-wrap with npm v3.5.2, did not get any errors. @koba04 let me know if this helped.

koba04 commented 8 years ago

@Scarysize Thank you for your work! I confirmed that npm-shrinkwrap works :tada:

mjrussell commented 8 years ago

Nice, sorry didn't realize there was some work to do packaging as well. Glad this worked.