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

docs: add minimum nodejs version in docs #1495

Merged
merged 8 commits into from
May 2, 2024

Conversation

inigomarquinez
Copy link
Member

@inigomarquinez inigomarquinez commented Apr 23, 2024

The purpose of this PR is to update the documentation to indicate the minimum nodejs version required for each express version, as required here.

I have added this information:

Warning

Please note that I have only made the change for the English version, as I don't know how translations of content into other languages are handled.

Captura de pantalla 2024-04-25 a las 8 23 46
Captura de pantalla 2024-04-25 a las 8 24 02
Captura de pantalla 2024-04-25 a las 8 24 15
Captura de pantalla 2024-04-25 a las 8 24 24

@inigomarquinez inigomarquinez changed the title docs: add minimum nodejs version in faq docs: add minimum nodejs version in docs Apr 25, 2024
@crandmck crandmck self-requested a review April 25, 2024 15:26
Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

LGTM! BTW this is related to expressjs/express#5595

@crandmck
Copy link
Member

crandmck commented Apr 26, 2024

Thanks for taking this on @inigomarquinez ! This is long overdue IMO... It will also close #1478.

However, I would ask that you make some changes. Rather than put the information in an FAQ, it should be stated right up front in "Installing".... So instead of

Check the minimum required Node.js for each Express version [here]

Just put the text you have in the FAQ now into "Installing". This is more direct, since the reader doesn't have to click the link, and the Node version is really a prerequisite so the reader needs to know it right up front.

Since this has been missing from the docs for so long, you can leave the FAQ for now. Otherwise, I would say it wouldn't be needed if you put it "Installing".

I have a couple other specific comments, as well, which I'll put inline.

en/5x/api.md Outdated
@@ -11,6 +11,8 @@ redirect_from: "/5x/api.html"

{% include note.html content="This is early beta documentation that may be incomplete and is still under development." %}

{% include note.html content="Express 5.0 will require Node.js 18 or higher." %}
Copy link
Member

@crandmck crandmck Apr 26, 2024

Choose a reason for hiding this comment

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

"Express 5.0 requires..."

Since 5.x is available now (in beta) it should be present tense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @crandmck !

Change added here

@@ -90,4 +90,11 @@ If you have a specific file, use the `res.sendFile()` function.
If you are serving many assets from a directory, use the `express.static()`
middleware function.

<a name="which-is-the-minimum-version-of-nodejs-that-express-supports"></a>

## Which is the minimum version of Node.js that Express supports?
Copy link
Member

Choose a reason for hiding this comment

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

"What version of Node.js does Express require?"

Elsewhere you use "require" instead of "support", which is preferable IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @crandmck ! I'm not a native English speaker, so I welcome these language suggestions.

Change added here.

@inigomarquinez
Copy link
Member Author

Thanks for taking this on @inigomarquinez ! This is long overdue IMO... It will also close #1478.

However, I would ask that you make some changes. Rather than put the information in an FAQ, it should be stated right up front in "Installing".... So instead of

Check the minimum required Node.js for each Express version [here]

Just put the text you have in the FAQ now into "Installing". This is more direct, since the reader doesn't have to click the link, and the Node version is really a prerequisite so the reader needs to know it right up front.

Since this has been missing from the docs for so long, you can leave the FAQ for now. Otherwise, I would say it wouldn't be needed if you put it "Installing".

I have a couple other specific comments, as well, which I'll put inline.

Thanks @crandmck !

I've pushed some new commits with your suggestions!

It now looks this way

Captura de pantalla 2024-04-30 a las 9 02 52

Captura de pantalla 2024-04-30 a las 9 03 07

@crandmck crandmck merged commit c0e3d78 into expressjs:gh-pages May 2, 2024
4 checks passed
@crandmck
Copy link
Member

crandmck commented May 2, 2024

Thanks @inigomarquinez !!

@inigomarquinez inigomarquinez deleted the docs/min-nodejs-version branch May 5, 2024 14:55
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.

3 participants