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

Create publicIp default export to return IPv6 (preference) or IPv4 (fallback) #59

Closed
wants to merge 33 commits into from

Conversation

Octalbyte
Copy link

No description provided.

@Octalbyte Octalbyte marked this pull request as draft December 27, 2021 19:15
@Octalbyte
Copy link
Author

Refers to #45

@Octalbyte Octalbyte marked this pull request as ready for review December 28, 2021 14:26
@Octalbyte
Copy link
Author

Octalbyte commented Dec 28, 2021

Thise PR is technically concluded. However, I think I will add documentation for the new feature. Actually, I need to extend the changes to the browser version :/

@Octalbyte
Copy link
Author

Sorry for all those "ava fix" commits.

@Octalbyte
Copy link
Author

Now the PR is ready for merge.

@sindresorhus
Copy link
Owner

While I appreciate the pull request, this is still far from being mergable. I recommend you look over your own code and you also need to add tests and TypeScript type updates.

.gitignore Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

sindresorhus commented Jan 25, 2022

#45 (comment)

@Octalbyte
Copy link
Author

@sindresorhus I've deleted the line mytest.js in the gitignore. Additionally, I've restructured the publicIp() function to use the Promise.any API. It is much simpler and cleaner now. I also added browser tests.
I am only not sure about:

  • How to add type definitions, now that publicIp is a function with other functions inside it
  • Which tests should be added for publicIp on node? Should they only run when the environment variable CI is false?
  • If an user has both IPv4 and IPv6, the IPv4 Promise might resolve sooner than the IPv6. And I found no safe way to make the thread sleep for some miliseconds, to give IPv6 an advantage.

.gitignore Outdated Show resolved Hide resolved
browser.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@Octalbyte
Copy link
Author

@Richienb I changed the PR according to requested. However, the re-request review button is not working for me.

@sindresorhus
Copy link
Owner

You also need to update the types, add docs, and add tests.

@sindresorhus
Copy link
Owner

I would recommend trying to figure out the questions you asked yourself.

@Richienb
Copy link
Contributor

If an user has both IPv4 and IPv6, the IPv4 Promise might resolve sooner than the IPv6. And I found no safe way to make the thread sleep for some miliseconds, to give IPv6 an advantage.

If IPv6 resolves first, cancel IPv4 but if IPv4 resolves first, wait for IPv6.

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

3 participants