-
Notifications
You must be signed in to change notification settings - Fork 22.8k
Update Express tutorial to v5 #40247
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
Conversation
files/en-us/learn_web_development/extensions/server-side/express_nodejs/introduction/index.md
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/mongoose/index.md
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/routes/index.md
Outdated
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/routes/index.md
Show resolved
Hide resolved
67404e6
to
f58bc20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamishwillee sterling work, sir. I've read through all the changes and made a few suggestions, but nothing major.
A couple of questions:
- Do you need me to run through it all and make sure it all works, or have you already done this with confidence? I'm assuming you have, but happy to help with this if not.
- I've not checked in detail whether you've missed any instances of mass replaced terms such as 4x -> 5x, but I'm assuming you've grepped for these?
...evelopment/extensions/server-side/express_nodejs/displaying_data/author_detail_page/index.md
Outdated
Show resolved
Hide resolved
..._development/extensions/server-side/express_nodejs/displaying_data/book_detail_page/index.md
Outdated
Show resolved
Hide resolved
...development/extensions/server-side/express_nodejs/displaying_data/genre_detail_page/index.md
Outdated
Show resolved
Hide resolved
.../learn_web_development/extensions/server-side/express_nodejs/forms/update_book_form/index.md
Outdated
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/introduction/index.md
Outdated
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/routes/index.md
Outdated
Show resolved
Hide resolved
files/en-us/learn_web_development/extensions/server-side/express_nodejs/routes/index.md
Show resolved
Hide resolved
.../en-us/learn_web_development/extensions/server-side/express_nodejs/skeleton_website/index.md
Outdated
Show resolved
Hide resolved
.../en-us/learn_web_development/extensions/server-side/express_nodejs/skeleton_website/index.md
Outdated
Show resolved
Hide resolved
@@ -345,11 +342,12 @@ The **package.json** file defines the application dependencies and other informa | |||
"debug": "~2.6.9", | |||
"express": "~4.16.1", | |||
"http-errors": "~1.6.3", | |||
"jade": "~1.11.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need a jade dependency if it is now called pug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I will fix
This pull request has merge conflicts that must be resolved before it can be merged. |
Co-authored-by: Chris Mills <chrisdavidmills@gmail.com>
119b976
to
f06dc0b
Compare
@chrisdavidmills - I have accepted all your changes, a couple with minor mods. Thanks so much.
I don't think this is necessary - though the offer appreciated! For the bits where code changes, mostly this was just removal of the
Yes it was a grep. Upshot, I am pretty confident of this. Should be good to go. mdn/express-locallibrary-tutorial#327 should merge at a similar time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamishwillee woo hoo — awesome work!
I'm approving this, but I won't merge it. I'll let you do that once the other related PR is ready to go too.
Thanks @chrisdavidmills . It's all good! |
This updates the Express Tutorial to v5
I wrote some tests in mdn/express-locallibrary-tutorial#327, then updated Express to v5 along with all dependencies - everything passed. Then ran all the code mods (
npx @expressjs/codemod upgrade
) suggested in the migration guide - nothing was changed. So I don't think I was using anything impacted by a changed or removed API.I have updated the docs now. Mostly this was removing code to do async error handling in routes using
asyncHandler
since Express now handles that automatically. Also updated the Express sections on handling errors so this is more clear.The route path handling is also different, so I had to update that section - it wasn't anything I was using in the demo, but it was significant for readers.
I also updated the database connection to to be done in
bin/www
since that allows for testing.The way it was done in the app.js means that tests can't work (because you can't have multiple connections, and if you do this in the app there is already a connection by the time you get to the test runner).
Last of all I went through the lot and checked all the docs and versions of libraries. I did not retest the deployment though (mostly because I did that fairly recently).
Actions:
Fixes #38922