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

New Node.js static server example #13113

Merged
merged 8 commits into from
Sep 28, 2022
Merged

New Node.js static server example #13113

merged 8 commits into from
Sep 28, 2022

Conversation

tshemsedinov
Copy link
Contributor

Summary

  • Totally rewrite example in a modern style
  • Add path traversal vulnerability issue
  • Remove half of MIME types not to overload example
  • Use file streams to reduce memory usage for big files

Motivation

Improve and renew legacy codebase.

Metadata

  • Adds a new document
  • Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

@tshemsedinov tshemsedinov requested a review from a team as a code owner February 18, 2022 14:26
@tshemsedinov tshemsedinov requested review from teoli2003 and removed request for a team February 18, 2022 14:26
@github-actions github-actions bot added the Content:Learn Learning area docs label Feb 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2022

Preview URLs

External URLs (1)

URL: /en-US/docs/Learn/Server-side/Node_server_without_framework
Title: Node.js server without a framework

(this comment was updated 2022-09-28 00:13:22.686039)

@hamishwillee
Copy link
Collaborator

@tshemsedinov This is excellent thanks.

@teoli2003 I've just done a basic subedit, not a technical review - left that for you.

@tshemsedinov
Copy link
Contributor Author

@hamishwillee thanks, I'll fix that in a few days, and can provide more simple examples of modern pure node.js and framework-agnostic approach.

Refs: https://github.com/mdn/content/discussions/13124

@Artuomka
Copy link

+1. Great idea!

@teoli2003 teoli2003 removed their request for review May 6, 2022 18:14
@hamishwillee
Copy link
Collaborator

Apologies @tshemsedinov - I did a subedit back in February but then thought I'd handed over to @teoli2003 for technical review. Obviously I did not do so properly

@teoli2003 Can you review and tell me if you are happy with this as a technical update?

My take on it is that it is a little "succint" for a learn article - but then so was the original. It lost some of the possible errors from the file read too. What I don't know is if this is a pattern that we would be OK for people to copy. Also whether the introductory statement is true - i.e. is this everything needed for a basic static file server.

@Josh-Cena
Copy link
Member

Josh-Cena commented Aug 12, 2022

I'm more familiar with Node than web (though I'm not a back end so I'm not that familiar with HTTP servers). The code here looks good enough to be copied—although it's definitely quite simple!


```js
const http = require('http');
const fs = require('fs');
Copy link
Member

Choose a reason for hiding this comment

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

Note: as a mid-term goal I'd like MDN Node tutorials to embrace Node ESM. More and more dependencies are migrating to ESM and you can't require ESM, which means for forward compatibility all user projects should start as ESM.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Josh-Cena: I think this is a great point. To give it more visibility, can you start a discussion about it (I believe the consensus will be easy to reach) at mdn/mdn-community ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I should—just haven't got to it yet...

Copy link
Member

@Josh-Cena Josh-Cena Aug 12, 2022

Choose a reason for hiding this comment

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

@tshemsedinov
Copy link
Contributor Author

@hamishwillee new example don't need to check if (error.code === 'ENOENT') because it checks is file exists before reading it and sends 404.html to the client with HTTP 404 code. So it completely repeats behavior of previous example and even extends it with big-files support but in a modern node way. I can extend this example with:

  • Memory cache to deduce reply latency
  • Reload cache watching for file changes
  • Generate directory index in html to select files
  • Support HTTP range requests

Listed will take about +10 lines each ☺

Comment on lines 20 to 21
<details>
<summary>CJS implementation</summary>
Copy link
Member

@Josh-Cena Josh-Cena Aug 13, 2022

Choose a reason for hiding this comment

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

Before we reached resolution with mdn/mdn-community#191, could you keep this CJS-only? This is simply confusing without another explanatory document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Josh-Cena done

@hamishwillee
Copy link
Collaborator

Thanks @tshemsedinov .

@Josh-Cena @teoli2003 I think we should get this in when you are happy. However it might be good to extend this in some or all of the ways outlined in #13113 (comment) as a post process - in particular if that can be done in separate sections such that a user can learn how each part is put together.

Do you have options about what you think such an example should do in the ideal case?

@estelle
Copy link
Member

estelle commented Sep 28, 2022

merging. If there are still issues, we can edit again.

@estelle estelle merged commit eb79c92 into mdn:main Sep 28, 2022
@charlesvg
Copy link

Providing an ESM version 👍

Assuming that from now on all codebases across the globe have migrated to ESM and not providing a CJS version equals to giving those who maintain legacy no other choice than to look elsewhere for a simple node http server.

PS, who performed a code review on this?
How semantic would you say the function name prepareFile is?
How readable would you say the function prepareFile is......?

@Josh-Cena
Copy link
Member

@charlesvg Passive-aggressive comments have no place on MDN and is borderline CoC violation.

The argument that there are people "maintaining legacy CJS applications" while needing to read learning materials to know how to write a simple HTTP server seems unlikely. We always assume people start from scratch and have no tech debt; otherwise it's hard for us to set any common ground.

The argument is also non-unique to ESM. You could ask: why does MDN use async/await? Why does MDN use at()? All of these syntaxes are well-supported in all Node LTS versions, so they are not experimental and we as editors have no reason to fear using them. If for some reason your environment doesn't support them, the solution is to use a transpiler/polyfill. Babel/tsc is almost inevitable on server-side as well, if you ever want to enforce type checking and/or extension syntaxes.

If you find prepareFile unreadable, feel free to send a refactor PR and let us review how it can be improved. The current code meets all engineering standards and is not less readable than other code examples on MDN. But ESM is a ship that's sailed—we'll not write new CJS code in our learning area examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:Learn Learning area docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants