Skip to content
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

Update route guide #1626

Open
wants to merge 6 commits into
base: gh-pages
Choose a base branch
from

Conversation

bjohansebas
Copy link
Member

No description provided.

Copy link

netlify bot commented Sep 21, 2024

Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit bc683e0
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/673a27158386a00008377b22
😎 Deploy Preview https://deploy-preview-1626--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bjohansebas bjohansebas requested review from a team September 21, 2024 01:11
en/guide/routing.md Outdated Show resolved Hide resolved
en/guide/routing.md Outdated Show resolved Hide resolved
en/guide/routing.md Outdated Show resolved Hide resolved
@@ -68,19 +68,24 @@ app.all('/secret', (req, res, next) => {

Route paths, in combination with a request method, define the endpoints at which requests can be made. Route paths can be strings, string patterns, or regular expressions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Route paths, in combination with a request method, define the endpoints at which requests can be made. Route paths can be strings, string patterns, or regular expressions.
Route paths, in combination with a request method, define the endpoints at which requests can be made. Route paths can be strings or regular expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about removing that part when I edited this file for the first time. Although it doesn’t work in Express 5, string patterns are present in version 4

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, I think "string pattern" is confusing... they're both strings. It's just Express 4 and 5 support strings of different syntax within the strings. I was thinking it'd be clearer to just note that the strings are different DSLs, and Express 4 happened to support regex characters, and if you wanted those characters literally they needed to be escaped.

Copy link
Member

Choose a reason for hiding this comment

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

It also depends on whether this doc is v5 first or v4 first I guess, but since the doc doesn't touch much on things that changed between the versions (i.e. * -> *name, etc) it might be easier to have it be things supported by each overall with caveats afterward for 4 vs 5 specifics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Although a note was added below stating that string patterns are not supported in version 5, so I'm not sure if it would be good to make the change you suggested.

If you want to make any further changes that you think are important to improve the router guide, it's enabled for maintainers to make edits. You know more about how the router works

Copy link
Member Author

Choose a reason for hiding this comment

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

cc: @expressjs/docs-wg

en/guide/routing.md Outdated Show resolved Hide resolved
Copy link
Member

@juliogarciape juliogarciape left a comment

Choose a reason for hiding this comment

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

LGTM

@UlisesGascon
Copy link
Member

Once the discussions are solve, I think that we can land it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants