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

Updated and refactored the security and performance best practices #1004

Closed
wants to merge 3 commits into from

Conversation

wesleytodd
Copy link
Member

@wesleytodd wesleytodd commented Nov 27, 2018

The security and performance recommendations were a bit out of date. While I was reading through it I felt like we really had a ton of things in here for which Express if not responsible for, and which us making recommendations for is questionable. Some of it I removed, some of it I refactored to just link out to other parts.

I also added a large section about measuring performance, because any discussion of best practices in performance is useless unless you measure it.

Lastly, there are a few sections which I have todo's left to resolve. But I had gotten to the point where I had too many changes and wanted some review and feedback before I wasted time perfecting it all.

Can you all review this and see if you think this is a good direction to go in before I spend tons of time finishing it up?

Edit: here is a link to view it in a reasonable format https://github.com/expressjs/expressjs.com/blob/perf-updates/en/advanced/best-practice-performance.md

@wesleytodd
Copy link
Member Author

FIY, the main changes are hidden by github because they are so large. The changes to the error handling page are really just parts which I moved from the performance and security section. I linked to them from in the existing page and made minimal updates.

@dougwilson
Copy link
Contributor

Thanks @wesleytodd ! Of course, in the end the change set is so large an true review is to be unlikely, haha. Anyone approving it would really just be saying "yea, it's probably fine".

That said, I'll still try to read through the PR as-is, but if you feel so inclined to break it up, that would be awesome. An example of breaking it up without (hopefully) too much work would be to identify one thing to do for example "Add Measuring Performance section" and just make a commit with that thing and you can then take this commit and smash it on top, without resolving any conflicts (i.e. just make all the files like they are in the commit currently). Iterating on that would result in digestable commits without requiring unwinding everything.

Doing that is optional, but appreciated 😅

@wesleytodd
Copy link
Member Author

Haha, yeah I agree it is much too large. I will break it up, it is just difficult because some of these sections were a bit interspersed. I think I can probably do 3 PRs, (measuring, in code, and in environment). I will update with that.

Copy link
Member

@crandmck crandmck left a comment

Choose a reason for hiding this comment

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

Thanks for keeping this article up-to-date @wesleytodd. Overall it looks good, but I made a few minor nitpicky grammar/spelling/usage type comments. After this lands, I'll try to make another "copy edit" pass some time just to clean it up a bit more--any edits I made would not change the content, and I'll give you a heads up if/when I do that.

en/guide/error-handling.md Outdated Show resolved Hide resolved
Using callbacks is one of the most common and robust ways to handle asynchronous activity in javascript code. By default it is up to the author of the function
to decide how errors are exposed, but luckily the node ecosystem has long since standardized on the "error first" pattern for callbacks. This pattern means that
almost all callback code will return an error or `null` as the first argument to a callback (for example `thing((err, result) => { /* ... */})`). Not only are
callbacks the most common way we handle error in node, it is also the way Express deals with them in the `next` callback.
Copy link
Member

Choose a reason for hiding this comment

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

"Not only are callbacks the most common way to handle errors in Node, they are also ..."

en/guide/error-handling.md Outdated Show resolved Hide resolved
en/advanced/best-practice-performance.md Outdated Show resolved Hide resolved
@dougwilson dougwilson force-pushed the gh-pages branch 20 times, most recently from bb4652e to 726bf05 Compare December 16, 2021 22:50
@dougwilson dougwilson force-pushed the gh-pages branch 2 times, most recently from 1fd61c6 to ef068f8 Compare February 18, 2022 03:09
Copy edits - grammar, spelling, etc.
copy edit: grammar and spelling.
@crandmck
Copy link
Member

crandmck commented Mar 4, 2024

I started to make the edits I requested (way back when) directly in the branch, but now I see that both of these files have been edited significantly since this PR was opened.

Rather than try and sort out all the conflicts, @wesleytodd , I suggest we close this and if you still want to make some of these changes, just open a new PR. OTOH if you really want to fix all the merge conflicts, please go for it and reopen.

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

Successfully merging this pull request may close these issues.

4 participants