Skip to content
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

Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 19, 2022

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 19, 2022
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2022
@bl-ue
Copy link
Contributor

bl-ue commented Apr 19, 2022

I'm not sure if it should be done in this PR, but I believe the following tests should be updated as well:

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 19, 2022

  • 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?

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
Copy link
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
Copy link

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

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@RaisinTen
Copy link
Contributor

RaisinTen 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.

+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
Copy link
Contributor Author

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.

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 21, 2022
@nodejs-github-bot 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 nodejs-github-bot merged commit 4ad342a into nodejs:master Apr 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 4ad342a

@aduh95 aduh95 deleted the fix-breaking-change-missed-out-docs branch April 21, 2022 23:00
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Refs: nodejs#41431
Fixes: 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]>
targos pushed a commit that referenced this pull request Apr 28, 2022
Refs: #41431
Fixes: #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]>
@targos targos mentioned this pull request May 2, 2022
@osiris-plus
Copy link

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

@aduh95
Copy link
Contributor Author

aduh95 commented Mar 29, 2023

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

@osiris-plus
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocumented change to "family" attribute in os.networkInterfaces() in Node 18