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

fixes #945 #946

Closed
wants to merge 3 commits into from
Closed

Conversation

mieszkomularczyk
Copy link

♻️ Current situation

This code does not work with nodejs 18, since the one field type in the objects returend by os.networkInterfaces() changed from string to number. 'family' field is either 4 or 6, previously it was a string IPv4 or IPv6

💡 Proposed solution

add extra check in the conditions so both old and new nodeJS work

@Apollon77
Copy link
Contributor

Sere there not also other places (in logging too) where this was used as string?

@Supereg
Copy link
Member

Supereg commented Apr 25, 2022

I'm not sure on the state of development of node v18. Is this a breaking change in node v18 which is here to stay? The linked comment on nodejs/node#41431 (review) sounded like it may be revised such that it doesn't affect current packages(?).

EDIT: Considering that development of node v18 just started and isn't even near its LTS state (and stuff like @types/node doesn't even exist yet), I'm not sure if we should rush to add this extra logic to all of our packages. Or what's your opinion on this?

@Apollon77
Copy link
Contributor

I read that one issue like "we want to do the change and should asjut affected libraries" ... eklse I would have expect3d the PR not to be accepted ... or?!

@oznu
Copy link
Member

oznu commented Apr 25, 2022

@Supereg it's probably worth doing sooner rather than later. There may be other compatibility issues in Node.js 18 we won't find until this one is patched.

@oznu
Copy link
Member

oznu commented Apr 25, 2022

@mieszkomularczyk

Maybe add some @ts-expect-error notices until we can get updated versions of @types/node? Using @ts-expect-error will ensure the exception is removed when it is no longer required.

      internal = true;
      // @ts-expect-error Nodejs 18+ uses the numbers 4 or 6 instead of the strings "IPv4" or "IPv6"
      if (info.family === "IPv4" || info.family === 4) {
        if (!ipv4) {
          ipv4 = info.address;
        }
      // @ts-expect-error Nodejs 18+ uses the numbers 4 or 6 instead of the strings "IPv4" or "IPv6"
      } else if (info.family === "IPv6" || info.family === 6) {
        if (info.scopeid) {
          if (!ipv6LinkLocal) {
            ipv6LinkLocal = info.address + "%" + name; // ipv6 link local addresses are only valid with a scope
          }
        } else if (!ipv6) {
          ipv6 = info.address;
        }
      }

@mieszkomularczyk
Copy link
Author

ok, it does not build anyway (kudos for the one who would explain why:) , therefore I close it until 18 reaches LTS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants