-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add notice for modern Node.js versions #30
base: master
Are you sure you want to change the base?
Conversation
README.md
Outdated
> [!NOTE] | ||
> This module is largely a wrapper of [`http.METHODS`](https://nodejs.org/api/http.html#httpmethods) from Node.js core with backwards compatibility for Node.js versions older than 0.11.8, making it mostly unnecessary for use in new projects. For newer Node.js versions, the following code is equivalent to using this module: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In npm, it doesn't follow the same format, and the styles that GitHub applies to this type of notes wouldn't be applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bummer, but would that be a reason to hold it up? I believe NPM will just show this as quoted text, which seems a reasonable fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can clarify this in the npm channel: https://openjs-foundation.slack.com/archives/CTEDKHCTB/p1729968334939499 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t it basically say this above already? Why not reword the exist text saying what the package is for? Existing text:
This module provides an export that is just like
http.METHODS
from Node.js core,
with the following differences
[…] * Contains a fallback list of methods for Node.js versions that do not have ahttp.METHODS
export (0.10 and lower).
Can we try to merge the redundancy in this description? Maybe link to the node docs instead of the code sample since you can write it at least 4 different ways nowadays depending on CommonJS, ESM, non-node prefixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Blake's proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've revised the notice not to repeat aforementioned statements and simply instruct the user to use the core module if their support target allows, sans code example.
Signed-off-by: Jon Koops <[email protected]>
6f3ed49
to
4b340af
Compare
Adds a notice to the README that explains to the user they might not need this module with example code that could replace the module in their project if they are targeting newer versions of Node.js.
See #29 for related discusssion.