Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
doc: fix outdated documentation for family property#42789
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
doc: fix outdated documentation for family property #42789
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Apr 19, 2022
Review requested:
|
bl-ue commented Apr 19, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I'm not sure if it should be done in this PR, but I believe the following tests should be updated as well: |
aduh95 commented Apr 19, 2022
FYI
Good catch, do you want to submit a PR?
AFAICT those are not related to this change (I've tried to update them in the original PR, but had to revert the changes because they were no longer passing). |
bnoordhuis commented Apr 20, 2022
Can I suggest to undo the string -> number change? It's caused at least one regression in a popular package (ref) and I expect it's not going to be the only one. |
davecarlson commented Apr 20, 2022
The change is fine, and is benefitial. It's in a Major release, and we are to expect some breaking changes (infact, majors are where the breakings are allowed!). |
ShogunPanda left a comment
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.
LGTM!
RaisinTen commented Apr 20, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
+1. I think we do breaking changes (even in major releases) only when the usage goes down in the npm ecosystem as a result of the runtime deprecation warning. |
aduh95 commented Apr 21, 2022
Note that reverting the string -> number change would be itself a breaking change at this point. If someone wants to revert it, I would advise them to open a PR ASAP, the more we wait the more users will start depending on the new behavior. |
nodejs-github-bot commented Apr 21, 2022
Landed in 4ad342a |
Refs: nodejs#41431Fixes: nodejs#42787 PR-URL: nodejs#42789 Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
Refs: #41431Fixes: #42787 PR-URL: #42789 Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>
osiris-plus commented Mar 29, 2023
@aduh95 the documentation website still shows outdated information: |
aduh95 commented Mar 29, 2023
The docs are correct, it's been reverted in v18.4.0 by #43054. |
osiris-plus commented Mar 30, 2023
I'm sorry, my distro is lagging behind it seems. |
I forgot to update the docs in a few places when working on #41431.
Fixes: #42787