Skip to content

Express connection logic didn't catch failed connections - #45492

Merged
Josh-Cena merged 3 commits into
mdn:mainfrom
hamishwillee:express_fix_connect_logic
Sep 4, 2026
Merged

Express connection logic didn't catch failed connections#45492
Josh-Cena merged 3 commits into
mdn:mainfrom
hamishwillee:express_fix_connect_logic

Conversation

@hamishwillee

Copy link
Copy Markdown
Collaborator

There was some non working connection logic in - a non-awaiting promise in a try-catch.

This was fixed and tested in mdn/express-locallibrary-tutorial#367

This fell out of #45473

@hamishwillee
hamishwillee requested a review from a team as a code owner September 4, 2026 02:23
@hamishwillee
hamishwillee requested review from dipikabh and removed request for a team September 4, 2026 02:23
@github-actions github-actions Bot added Content:Learn Learning area docs (outside of one of the Learn:* subtags) size/s [PR only] 6-50 LoC changed labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Preview URLs (1 page)

(comment last updated: 2026-09-04 07:23:16)

@hamishwillee
hamishwillee marked this pull request as draft September 4, 2026 02:26
The bin/www generated by express-generator starts the HTTP server
unconditionally, so simply fixing the connectMongoose() try/catch
logic wasn't enough: readers following the tutorial would still end
up with a server that starts regardless of whether the database
connected. Move server creation into a startServer() function that
only runs once connectMongoose() resolves, matching the upstream fix
in mdn/express-locallibrary-tutorial#367.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018BoWRns9KXjAdyMZMeQ4g2
@github-actions github-actions Bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Sep 4, 2026
@hamishwillee
hamishwillee marked this pull request as ready for review September 4, 2026 02:31
@hamishwillee

Copy link
Copy Markdown
Collaborator Author

Note, every time I need to make an update like this I start coming around to the idea that we should stop using the generator and create our own template starting point. What I'd really like is an expert to update the current generator :-)

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions Bot added the merge conflicts 🚧 [PR only] label Sep 4, 2026
@Josh-Cena

Copy link
Copy Markdown
Member

Note, every time I need to make an update like this I start coming around to the idea that we should stop using the generator and create our own template starting point. What I'd really like is an expert to update the current generator :-)

Agreed!! Well I might be investing in #41604. Stay tuned.

@github-actions github-actions Bot removed the merge conflicts 🚧 [PR only] label Sep 4, 2026
Comment on lines +733 to +741
var server;

function startServer() {
server = http.createServer(app);

server.listen(port);
server.on("error", onError);
server.on("listening", onListening);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I hate this so much. Violates a bunch of code style guidelines. But I guess you really don't have better choices at this point.

I'll make starter code of our own.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe fork the code generator. There is a lot to be said for automated setup.

@Josh-Cena Josh-Cena left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamping since you already have another PR

@hamishwillee

Copy link
Copy Markdown
Collaborator Author

Rubber stamping since you already have another PR

Screw the other PR. If you want to fix this, propose a replacement. I took the easy road because to me this is just boilerplate.

@Josh-Cena

Copy link
Copy Markdown
Member

Yes I'm happy with this 😄

@Josh-Cena
Josh-Cena merged commit 3d04bc6 into mdn:main Sep 4, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Content:Learn Learning area docs (outside of one of the Learn:* subtags) size/m [PR only] 51-500 LoC changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants