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

fix(gatsby-telemetry): use windowsHide to not show windows command prompt windows #28258

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 24, 2020

Description

This is quite weird issue - sometimes on Windows when using default "Command prompt" there would be cmd.exe windows popping up and immediately hiding (see videos in #28233 both from issue author and from me).

I used https://docs.microsoft.com/en-us/sysinternals/downloads/procmon to track down what those actually try to execute and found that it's our repository id getter function:
Screenshot 2020-11-24 at 13 40 45

childProcess.execSync (and all the other similar too) have windowsHide option that is disabled by default, which I flip here to handle the issue.

Seperate issue is that we call it multiple times - just from different processes, so memoization we have in

getRepositoryId(): IRepositoryId {
if (!this.repositoryId) {
this.repositoryId = _getRepositoryId()
}
return this.repositoryId
}
doesn't address cross process ( which might be fine not to handle, just good to be at least aware of it )

And cherry on cake here is that at least for me - without this windowsHide toggle - I only see those command prompts windows popping up in some cases - in here it seems like after main gatsby develop process dies / get killed

Related Issues

Fixes #28233

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 24, 2020
Copy link
Contributor

@jamo jamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! (do we want the same to flush.ts?)

@pieh pieh added topic: telemetry* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 24, 2020
@pieh
Copy link
Contributor Author

pieh commented Nov 24, 2020

Nice catch! (do we want the same to flush.ts?)

We might, but we need to be careful about things like - I encountered sindresorhus/execa#433 when doing preliminary investigation for this issue (which turned out absolutely not related to execa, but first comment does touch on some potential issues with windowsHide, which TBF I didn't get deep into it other than "ok this has potential for issues, don't touch more things than absolutely required without a LOT more research into this"). I think we should be fine here with git execution because this is short command that exits on itself quickly (and also have timeout setting).

So for this PR I think we should not add anything more. Just keep this in back of our heads whenever we encounter another issue of Windows Command Prompt window popping up and disapearing.

@pieh pieh merged commit e416368 into master Nov 25, 2020
@pieh pieh added this to To cherry-pick in V2 Release hotfixes via automation Nov 25, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/telemetry-phantom-command-prompts branch November 25, 2020 07:59
@jamo
Copy link
Contributor

jamo commented Nov 25, 2020

Thanks!

vladar pushed a commit that referenced this pull request Nov 25, 2020
vladar added a commit that referenced this pull request Nov 25, 2020
…ompt windows (#28258) (#28285)

(cherry picked from commit e416368)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
@vladar vladar moved this from To cherry-pick to Backported in V2 Release hotfixes Nov 25, 2020
@vladar
Copy link
Contributor

vladar commented Nov 25, 2020

Published in gatsby@2.27.3 and gatsby-telemetry@1.5.1

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

Successfully merging this pull request may close these issues.

Popped up command window without saving
3 participants