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

dep!: replace os-name with systeminformation #84

Closed

Conversation

erisu
Copy link

@erisu erisu commented Jan 27, 2023

resolves #83

PR Changes

  • Remove os-name
  • Add systeminformation

Summary

What is your opinion on replacing the os-name dependency with something else that can also get the operating system name?

This PR changes the dependency to systeminformation which can also get the name.

The downside

  • The dependency is bloated. The unpacked size is about 731.4 kB.
  • It has a ton of APIs for fetching various information that we most likely will never use. It is possible that we could provide in the future an ability for allowing users to configure Insight on what additional information they want to collect.
  • It is also asynchronous.

The plus side

  • It appears to provide a more detailed name for Linux. E.g. the distro name Debian, RedHat, FreeBSD, etc.
  • Does not require any extra dependencies. Uses only what comes with node. E.g. os, child_process, etc
  • Says to work with node >=8.

Testing

In terms of testing, I only ran the unit tests and it passes.

Final Thoughts

I believe it might be enough, but would like a review and if anyone can confirm that GA receives the data they expect.

If the bloating is an issue, maybe we could ask and see if they could extract it as a module so we could selectively pick what we want.

systeminformation basically has similar information as os-name and similar mapping as macos-release library that os-name uses. The plus side is it contains a fallback value to ensure that it will not crash with future releases of macOS.

@erisu erisu closed this May 9, 2023
@erisu erisu deleted the feat/drop-os-name-use-systeminformation branch May 9, 2023 03:36
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.

Support macOS 13 Ventura
1 participant