expressjs.com icon indicating copy to clipboard operation
expressjs.com copied to clipboard

Updated and refactored the security and performance best practices

Open wesleytodd opened this issue 5 years ago • 3 comments

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 avatar Nov 27 '18 02:11 wesleytodd

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.

wesleytodd avatar Nov 27 '18 02:11 wesleytodd

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 😅

dougwilson avatar Nov 27 '18 03:11 dougwilson

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.

wesleytodd avatar Nov 27 '18 19:11 wesleytodd

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.

crandmck avatar Mar 04 '24 00:03 crandmck