FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: fix outdated documentation for `family` property by aduh95 · Pull Request #42789 · nodejs/node · GitHub

/ node Public

doc: fix outdated documentation for family property - #42789

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
aduh95:fix-breaking-change-missed-out-docs
Apr 21, 2022
ConversationCommits (1)ChecksFiles changed
Merged

doc: fix outdated documentation for family property#42789
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
aduh95:fix-breaking-change-missed-out-docs

Conversation

aduh95 commented Apr 19, 2022

Copy link
Copy Markdown
Contributor

I forgot to update the docs in a few places when working on #41431.
Fixes: #42787

nodejs-github-bot commented Apr 19, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 19, 2022
aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 19, 2022

bl-ue commented Apr 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author
  • test/parallel/test-net-remote-address-port.js

FYI remoteFamily is still returning a string (which is arguably confusing, but I also didn't want to introduce another unnecessary breaking change).

  • test/parallel/test-dgram-udp6-link-local-address.js

Good catch, do you want to submit a PR?

node/test/parallel/test-dgram-udp6-link-local-address.js

Line 15 in 1e76165

if (family === 'IPv6' && address.startsWith('fe80:')) {

  • test/internet/test-dgram-multicast-set-interface-lo.js
  • test/common/report.js

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown

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.

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!).
If it's an issue, stay on 16/17 until deps are updated

ShogunPanda approved these changes Apr 20, 2022
View reviewed changes

ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

RaisinTen commented Apr 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

+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

Copy link
Copy Markdown
Contributor Author

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.

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2022
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2022
nodejs-github-bot merged commit 4ad342a into nodejs:master Apr 21, 2022

nodejs-github-bot commented Apr 21, 2022

Copy link
Copy Markdown
Collaborator

Landed in 4ad342a

aduh95 deleted the fix-breaking-change-missed-out-docs branch April 21, 2022 23:00
VoltrexKeyva mentioned this pull request Apr 25, 2022
Closed
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
doc: fix outdated documentation for family property
20e163a 
Refs: nodejs#41431
Fixes: nodejs#42787

PR-URL: nodejs#42789
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this pull request Apr 28, 2022
doc: fix outdated documentation for family property
d9f3f05 
Refs: #41431
Fixes: #42787

PR-URL: #42789
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos mentioned this pull request May 2, 2022
Merged

osiris-plus commented Mar 29, 2023

Copy link
Copy Markdown

@aduh95 the documentation website still shows outdated information:
https://nodejs.org/docs/latest-v18.x/api/os.html#osnetworkinterfaces

aduh95 commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

The docs are correct, it's been reverted in v18.4.0 by #43054.

osiris-plus commented Mar 30, 2023

Copy link
Copy Markdown

I'm sorry, my distro is lagging behind it seems.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ShogunPanda ShogunPanda approved these changes
lpinca lpinca approved these changes
BethGriggs BethGriggs approved these changes
+1 more reviewer
Mesteery Mesteery approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Undocumented change to "family" attribute in os.networkInterfaces() in Node 18
Back | FazBrowse Home | New Git URL