Upgrade Express to v4 #30
Conversation
kevinburkeshyp
force-pushed
the
upgrade-express
branch
from
7c3b8d5
to
e9a915c
Compare
|
It's also worth noting that Sails core rejected the Express 4 upgrade in favor of calling all of the dependency modules ( |
|
@kevinburkeshyp Can I make a process request in the future – if there are members of the team who have expressed interest in changing or knowing about particular parts of the system, could you FYI/ping them on PRs that make those changes? For example, I'm very interested in this change (and will review this later today), but would have easily missed the update in Slack if I hadn't happened to see the mention in the #platform channel. Thanks! |
|
Sure - I also tried to ping you in #growth-team |
|
Oh, my bad – I see that you did ping me there. Thank you! |
| // This is so that all the built-in core Express/Connect middleware | ||
| // gets called before matching any explicit routes or implicit shadow routes. | ||
| router: app.router, | ||
|
|
||
| // Add powered-by Sails header | ||
| poweredBy: function xPoweredBy(req, res, next) { |
There was a problem hiding this comment.
Can we turn this off? It's a potential security hole to announce the server software. Express4 (and maybe 3 too) allow it to be disabled with app.set('x-powered-by', false) (see docs).
There was a problem hiding this comment.
yep totally fine by me, I'll do in separate PR
|
@kevinburkeshyp Thank you for doing this. |
kevinburkeshyp
force-pushed
the
upgrade-express
branch
from
e9a915c
to
f5ead59
Compare
Not yet, I guess I can run the API tests against this branch and see what passes/fails. I was going to try and get this through first. |
kevinburkeshyp
force-pushed
the
upgrade-express
branch
from
2c85756
to
550f78a
Compare
| @@ -31,10 +31,12 @@ | |||
| "captains-log": "0.11.11", | |||
| "colors": "0.6.2", | |||
| "commander": "2.1.0", | |||
| "compression": "^1.6.1", | |||
|
One comment, otherwise this looks like 🍧 to me. |
kevinburkeshyp
force-pushed
the
upgrade-express
branch
from
550f78a
to
f7dd603
Compare
This was accomplished by installing Express 4, attempting to run the tests, observing things that broke, and then fixing them. In some cases, I leaned on the work done in balderdashy#3235 to figure out how to do something. The most comprehensive change is to the router, which is its own Router object, and no longer a function on an Express `app`. Adds two new dependencies (which were removed from Express core): cookie-parser and compression. In some cases we removed the dependency on Express instead of upgrading - we no longer try to serve favicons, or deal with sessions.
kevinburkeshyp
force-pushed
the
upgrade-express
branch
from
f7dd603
to
39317fb
Compare
This was accomplished by installing Express 4, attempting to run the tests,
observing things that broke, and then fixing them. In some cases, I leaned on
the work done in balderdashy#3235 to figure out
how to do something. The most comprehensive change is to the router, which is
its own Router object, and no longer a function on an Express
app.Adds two new dependencies (which were removed from Express core): cookie-parser
and compression. In some cases we removed the dependency on Express instead of
upgrading - we no longer try to serve favicons, or deal with sessions.