Update dependencies with david #787
Conversation
| @@ -8,11 +8,13 @@ | |||
| "nuke": "asini clean && rm -r node_modules", | |||
| "changelog": "asini-changelog", | |||
| "debug": "cat asini-debug.log && for d in packages/*/npm-debug.log*; do echo $d; cat $d; done", | |||
| "postinstall": "npm run bootstrap-no-i" | |||
| "postinstall": "npm run bootstrap-no-i", | |||
| "update-dependencies": "asini exec david u" | |||
There was a problem hiding this comment.
Should this also run david on the repo root? Then we could disable the GreenKeeper integration and just manage everything with david?
|
Wow, thanks for dealing with this @doug-wade! This seems like a generally pretty high-risk change... 🤔 Any additional testing we should do for this? |
|
(Edited the title to reflect that this PR actually updates deps) |
doug-wade
force-pushed
the
do-you-trust-your-ci
branch
from
27dfd01
to
2499aa9
Compare
|
@gigabo I've done all the testing I think I can -- it's passing ci, I linked up all the examples and ran them, and I ran through the test pages. I agree there are risks, but I think we should try to keep the ecosystem up-to-date as much as reasonable. |
|
@doug-wade Thanks for manually regressing. Bombs away! 💣 |
| "routr": "0.1.3", | ||
| "winston": "2.2.0" | ||
| "request-local-storage": "^1.1.1", | ||
| "routr": "^2.1.0", |
There was a problem hiding this comment.
@doug-wade @gigabo The upgrade of routr from version 0.1.3 to >1.0 introduced some breaking changes that impact us. These changes and the bug in #312 are related, so I'll track it there.
There was a problem hiding this comment.
how do those changes affect us? and why didn't the automated tests catch the problems?
There was a problem hiding this comment.
See #837 and #312. The change had a few impacts:
- Routr no longer does anything with the
navigateoption in https://github.com/redfin/react-server/blob/master/packages/react-server/core/context/Navigator.js#L53. I have NO idea what thetypevariable is really referring to in our code...it seems super weird to me. The Routr project said in their commit message: "If needed, the navigation logic should happen at a higher level or remove that special case." - According to Requests other than "get" is not working #312, we have never been passing the HTTP method information to Routr anyway, so Routr never checked the method properties.
- We are improperly encoding the page attribute
methodin theroutes_(client|server).jsfile because our documentation says it can be an array or string (Routr now supports an array), but we're encoding it as a string only.
There was a problem hiding this comment.
The tests didn't catch this because all of our tests only send GET requests, no POST/HEAD/etc. I added some unit tests in #837, but we should probably add full integration tests for at least POST requests.
There was a problem hiding this comment.
Let me rephrase this. The breaking change in Routr may or may not impact us. It all has to do with what we use/expect regarding the type property on this line. Our tests might not catch this for two reasons: we don't have test coverage of this case or the change in Routr actually doesn't impact us.
Secondly, the bug identified in #312 means that we're not even passing the HTTP method to Routr for it to check. The fix for their API change and #312 is to pass the HTTP method to Routr. That change is a breaking change for our project because now people will have to allow POST requests in their routesFile or else Routr won't select them.
Since we merged #732, greenkeeper hasn't been keeping our deps up-to-date, and it never updated our packages dependencies. Use david to update our dependencies instead, and upgrade most of them (
chunk-manifest-webpack-pluginis for webpack 2, for instance) to the most recent available version