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

Fall back to using environment variables when os.hostname fails #5

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

yousif-bugsnag
Copy link

v3.0.1 added a simple guard around os.hostname to prevent a crash on Windows 7 - however the original library authors have correctly pointed out that this significantly increases the likelihood of collisions on Win 7 - paralleldrive#264 (comment)

This is a port of @FlyingDR's PR on the original cuid library (paralleldrive#264) which improves that fix by attempting to fall back to environment variables when os.hostname fails.

@FlyingDR
Copy link

@yousif-bugsnag As an author of the pull request in the original library I need to mention that the comment from the library author about the increase of collision likelihood was given for an initial version of the patch, which was later updated.

Reaction of the library author to the updated version was much more positive.

@yousif-bugsnag
Copy link
Author

@FlyingDR yes absolutely - sorry if my PR comment wasn't very clear in that regard. Our current fix in v3.0.1 of @bugsnag/cuid mirrors the initial version of that patch, which increases the chance of collisions on Windows 7. This PR is a port of your improved/updated fix which addresses that.

And thank you for all your efforts and contributions on this issue!

@FlyingDR
Copy link

Glad to see it as a PR for Bugsnag :)

bugsnag/bugsnag-js#1783 is worth mentioning here since it clearly depends on this PR

@yousif-bugsnag yousif-bugsnag merged commit 526a418 into master Nov 25, 2022
@yousif-bugsnag yousif-bugsnag deleted the win7-hostname-fallback branch November 25, 2022 17:20
@yousif-bugsnag yousif-bugsnag mentioned this pull request Jan 9, 2023
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.

3 participants