Clarify README grammar slightly #353
Conversation
|
Thanks for this! I'll review in the next 2 weeks or so (things are a bit busy for me right now). |
There was a problem hiding this comment.
I think both the original and your amendment aren't 100% clear. Thanks for bringing this to my attention!
I'm trying to convey the following things:
- If you're not using Express, this middleware will remove the
X-Powered-Byheader. - If you are using Express, this middleware will remove the
X-Powered-Byheader, but you'd be better off using Express's built-in functionality for doing this:app.disable("x-powered-by"). But it doesn't really matter.
How about this instead:
Note: Express has a built-in way to disable the
X-Powered-Byheader, which you may wish to use instead of this middleware.
Would that be clearer?
Just adding my two cents here, but the verbiage here has confused me also in the past; to be honest, I would often just use the Helmetjs method and still disable it natively. But yes, @EvanHahn, I find the version you suggested to be the clearest of them all. |
|
@zachhardesty7 Could you change it to: Note: [Express has a built-in way to disable the `X-Powered-By` header](https://stackoverflow.com/a/12484642/804100), which you may wish to use instead of this middleware.If I don't hear back in a few days, I'll just do it myself (which is no problem). |
|
Did this myself in dbcf9c4. Thanks for bringing up the discussion! |
|
@EvanHahn, thank you for doing that for me, hadn't gotten a chance to check back until now! Ah yes, given the actual intended meaning, my revision was sorta incorrect and your commit is much clearer and makes perfect sense. Just a quick note, you said "you'd be better off" with the Express version, which is not indicated in the revised text. I assume since you said it doesn't really matter, it's not important to recommend one way over the other? This is certainly somewhat pedantic, but since we're already looking at the wording, might as well. |
|
I think it basically doesn't matter which one you choose. From a performance perspective, I believe the effect is negligible. I created a simple benchmark app and couldn't find a significant performance difference between the two. It's possible that a more robust benchmark would find a difference, but I doubt it. From a usability perspective, I also don't think it matters much. Helmet's goal is to be easy to drop in, and I think people can better spend their time on other tasks. If I start highlighting (what I believe to be) a tiny difference in the docs, I fear it might cause confusion. Does that seem reasonable? |
|
Agreed. If there is no performance difference and the verbiage is a bit clearer now, great. But we certainly have more important things to do than critiquing semantics, generally speaking. |
|
Perfect. If this decision (or any other) seems unclear or wrong, please let me know and we can fix it! |
This is a tiny change to adjust the wording from "instead" to "otherwise". I believe this is congruent with the intended meaning of the sentence, but if not, clarification would be appreciated. As a native US English speaker, this sentence confused me for at least a few minutes.
Slightly more technically, the combination of "but" and "instead" here implies (to me) that the preceding clause ("this middleware will work") is unconditionally negated by "you should use
app.disable("x-powered-by")". It implies that you should always prefer usingapp.disable("x-powered-by")instead of this middleware.I chose not to in the PR, but it could also be written more simply as "If you're not using Express, you should use
app.disable("x-powered-by")." It's previously documented that Helmet is a tool for Express, so the reader already knows this middleware works for Express.