-
Notifications
You must be signed in to change notification settings - Fork 993
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
fix(telemetry): Enable telemetry on windows #7389
Conversation
Ping: @thedavidprice |
…od into jgmw-telemetry-improvise
// On windows process.argv may not return an array of strings. | ||
// It will look something like [a,b,c] rather than ["a","b","c"] so we must stringify them before parsing as JSON | ||
// "os.type()" returns 'Windows_NT' on Windows. See https://nodejs.org/docs/latest-v12.x/api/os.html#os_os_type. | ||
if (os.type() === 'Windows_NT') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this package already used os.type
, but the rest of the codebase seems to use process.platform
. Any benefit of using one or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not immediately aware of anything specific other than if we use process.platform
it would be one less require? @cannikin had a specific reason behind this choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably didn't realize there even was a process.platform
! Is there anything else telemetry uses from that os
lib that isn't available through process
?
Also the strings that process.platform
returns may be different than what's returned by os
? We would want to either convert them to be the same, or update the existing entries in the telemetry database to match, so we don't have different strings for the same platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that os
is only used for the is windows conditionals.
The actual payload that's sent as telemetry is constructed from data from the envinfo
package. That would mean no changes to the actual payload and so no data discontinuities. Unless I'm mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Yeah let's axe it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this https://stackoverflow.com/a/61744810/88106
Seems to suggest os.type()
might be more reliable in VMs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node docs https://nodejs.org/api/os.html#osplatform for os.platform()
state that "The return value is equivalent to process.platform."
Although not sure if that SO comment was highlighting that that isn't reliable when operating in VMs.
EDIT: I guess we could just leave it as os.type()
. It's worked this long without issues and it appears from the docs that Tobbe's right about being more reliable on VMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out I was a genius all along! 😅
Does that mean we should change the existing uses of |
* Update spawn options and argv processing for windows * comment typo
Fixes #5901
Problem
Telemetry is disabled on windows due to the background process spawning console windows. This is too intrusive for users.
Changes
process.argv
options don't return a string which is a valid JSON string array. They return something like[abc,def,xyz]
rather than["abc","def","xyz"]
. We therefore need to pre-process this before passing it to yargs.Outstanding
Notes
yarn rw info
calls it was me.