lib: Rebrand getAtomAppName() function (fix shelling out to `git` on macOS) #13

DeeDeeG · 2023-01-17T16:49:03Z

[ EDIT: this PR applies to macOS only ]

Fixes pushing to a git remote, and probably fixes shelling out to the system's git command in general.

Note: I used the branding API, so if anyone forks Pulsar, this bit should hopefully still work for them. But if the branding API doesn't work/isn't available for whatever reason, this falls back to just return Pulsar Helper.app (hard-coded). It was simple enough to do it this way, so I figured, "why not?"

Fixes pushing to a git remote, and probably fixes shelling out to the system's `git` command in general.

DeeDeeG · 2023-01-17T16:58:40Z

Tangential Note: I would like anyone reviewing this to also consider #12 as well, since I'd like the repo code to work on master branch.

If we are going to only include this package in the app after compiling to a dist folder and tagging a release, then #12 can maybe be ignored. Makes it a little harder to hack on the package with ppm link though.

savetheclocktower · 2023-01-22T19:33:01Z

probably fixes shelling out to the system's git command in general.

Can you explain what is currently not working properly with shelling out to git? I ask because I'm noticing some stuttering/locking of the sort described in this document (even though that document talks about how they avoid those effects) and it seems to be because of a large number of shellouts to git that happen whenever I focus the tab that has my snippets file.

I came here to file this as an issue, but I don't want to create a new issue if this PR somehow addresses that behavior. Thanks!

DeeDeeG · 2023-01-23T19:10:09Z

Hi @savetheclocktower,

It seems to me this PR has an unrelated purpose to the performance issues you're describing. If you do want to open an issue, it would be appreciated, and hopefully someone will be able to narrow down the cause and what the fix would be.

I can go ahead and describe what this PR fixes:

In Electron on macOS, there is a main Electron binary, and a "helper" binary. This PR addresses a part of the github package where a path had been hard-coded to assume this helper's name is "Atom Helper.app", whereas in our case it will be "Pulsar Helper.app".

Anything in this package that requires this "helper" binary in Pulsar will fail without the current PR due to "Pulsar.app/.../Atom Helper.app" not existing. The only use I am aware of in this package is shelling out to the CLI git command on the user's PATH. (Perhaps there are other uses of this path as well, I am mostly doing this to solve the issue where git push doesn't work.)

Thank you!

EDIT TO ADD:

You may want to try this, from the link you posted?

If you wish to see the sidecar renderer process window with its diagnostic information, set the environment variable ATOM_GITHUB_SHOW_RENDERER_WINDOW before launching Atom. To opt out of the sidecar process entirely (for CI tests, for example) set ATOM_GITHUB_INLINE_GIT_EXEC.

DeeDeeG · 2023-01-23T19:15:17Z

@pulsar-edit/core can I get a review of this? This fixes a user-facing issue where pushing branches/etc. doesn't work from the github package in Pulsar.

Spiker985

Optional chaining - I like it

Something to note - this function only exists in GitHub - but there is a similarly named function getAppName() which is used elsewhere

Why they had two functions for the same result - I have no clue

confused-Techie

Love use of the optional chaining, the only thing I'd recommend here (But not at all needed) would instead of checking for a falsy value with || I'd change that to use the Nullish Coalescing Operator ?? to check for strictly null or undefined on the left hand operator.

Like I said not at all needed, and only really provides a benefit if some weird stuff ends up in the name that we do want to show like an empty string which would evaluate as falsy

Otherwise approve and say we should merge!

DeeDeeG · 2023-01-24T03:26:19Z

Nullish Coalescing Operator ??

only really provides a benefit if some weird stuff ends up in the name that we do want to show like an empty string which would evaluate as falsy

Maybe we need to clarify about the branding API that the values in there (at least what we have so far) should definitely only be non-empty strings, since I feel like not doing that could break a bunch of other things throughout Pulsar...

Hmm.

Like I'm pretty sure we do string-only operations, or similar falsy checks as I did here, and package authors/fork developers should be able to do the same with confidence, IMO.

Looking closer at this particular usage

Snippet where this is actually used:

github/lib/helpers.js

Lines 95 to 99 in 6fc9e12

    
           if (process.platform === 'darwin') { 
        
             const appName = getAtomAppName(); 
        
             return path.resolve(process.resourcesPath, '..', 'Frameworks', 
        
               `${appName}.app`, 'Contents', 'MacOS', appName); 
        
           } else {

(And note: getAtomAppName() should really be called getAtomHelperName()... It's not a general-purpose "what's the app name?" it's quite specific, really. And it's not exported outside this file, it's just a helper function to getAtomHelperPath(), which is the one that's exported and used throughout the package...)

This is used to find a file path on disk of the (rebranded) Electron Helper binary, should be: /Applications/[Your app name].app/Contents/Frameworks/[Your app name] Helper.app/Contents/MacOS/[Your app name] Helper.

Such as: /Applications/Pulsar.app/Contents/Frameworks/Pulsar Helper.app/Contents/MacOS/Pulsar Helper

If we're getting something falsy (but not null or undefined I guess), that path's gonna be pretty weird, and I dunno what the idea is naming one's app anything remotely like a JS falsy value. I guess your atom.branding.name is like JS 0 (again, not a string "0" but a JS Number type value 0) or something weird like special JS value NaN, or like you said, someone put the empty string "" in there, and at that point, why? Misusing the API big-time IMO. I have to assume we do some string-only operations on the branding API values in various places?

DeeDeeG · 2023-01-24T03:36:41Z

Actually, I'm pretty sure there is an ~~Electron~~ Atom API for this that might be more appropriate? It's app.appName... Unfortunately for us, in the Beta releases that's currently "Pulsar Beta". Which... Isn't quite right. 💀 So, not that, at least not for now.

[ EDIT: never mind, the app.getName()Electron API method (https://www.electronjs.org/docs/latest/api/app#appgetname) is main-process only, and named slightly different, so from the renderer process (and Dev Tools) atom.getAppName() must be an Atom API, not directly the Electron one. ]

But so far I think we only use the branding API for cosmetic stuff, not "business logic"?

DeeDeeG · 2023-01-24T03:38:53Z

Sorry if my two replies above are off-topic to ?? vs ||, but I don't think ?? should help here (or should be allowed to help here -- falsy values would be a use of the branding API which IMO should not be supported).

I am personally against using ?? here, since I think when I look at ?? my exact initial reaction is "??". So I would prefer || wherever the two are equivalent, and ?? should be used sparingly, IMO. Solely based on my personal opinion. || is more readable to me, personally.

confused-Techie · 2023-01-24T04:29:49Z

Sorry if my two replies above are off-topic to ?? vs ||, but I don't think ?? should help here (or should be allowed to help here -- falsy values would be a use of the branding API which IMO should not be supported).

I am personally against using ?? here, since I think when I look at ?? my exact initial reaction is "??". So I would prefer || wherever the two are equivalent, and ?? should be used sparingly, IMO. Solely based on my personal opinion. || is more readable to me, personally.

Yeah that's totally fair. Like I said not required in any way. Guess I've spent to much time doing server side stuff and am prepared to get weird data from users, in which case in a situation like this the backend has to prepare to get weird stuff like an empty string back from whatever condition, and we failover only in cases where it's properly a null value.

But you are right, in this situation there is likely a lot else going wrong if the branding API is returning string based falsy values.

So yeah no worries, otherwise totally approve the PR.

DeeDeeG · 2023-01-24T04:48:47Z

I convinced myself to do the nullish after all, as someone intending to call their app 0, as in the digit zero, ~~probably has~~ [ make that: "could somewhat dubitably" ] have their heart in the right place, so we can respect the intent (again, somewhat dubitably?) inferred from their use of the branding API.

If anyone forks Pulsar and names their app 0: please make that the string version, "0" when using the branding API, but also: you're welcome.

(That said: If anyone forks Pulsar and makes the branding API values the empty string "", or NaN, or false or BigInt zero 0n, you're not welcome, and please don't! (lol.) Per: https://developer.mozilla.org/en-US/docs/Glossary/Falsy. I don't wanna see these being used in the wild in the branding API, please and thank you!)

Per review suggestion in the PR. This treats falsy but non-nullish inputs, such as Number 0, as valid. Other than digit zero `0`, I can't think of a falsy JS value that seems like a valid app name, so they're *probably* misusing the branding API at this point, but this is for whoever uses `0` as their `branding.name`. (Note: Please use string `"0"`, or any non-empty string for that matter, instead!)

DeeDeeG · 2023-01-24T06:29:43Z

Okay, thanks for the reviews, folks! Merging this!

Hopefully will either get a couple more small fixes in to the github package and then tag and do a PR to get them into pulsar-edit/pulsar... Or if I don't get around to those other fixes for this package, maybe just get this bug fix out to pulsar-edit/pulsar real quick.

Should be soon ™️!

confused-Techie · 2023-01-24T06:40:37Z

@DeeDeeG Glad to see you were convinced in the end aha. It isn't as clear to read, but once familiar with it becomes super helpful.

Especially when you extrapolate the ? character from the optional chaining, and ternary operators it solidifies ? meaning left operand nullish. At least for me

lib: Rebrand getAtomAppName() function

fdb0466

Fixes pushing to a git remote, and probably fixes shelling out to the system's `git` command in general.

Spiker985 approved these changes Jan 23, 2023

View reviewed changes

confused-Techie approved these changes Jan 24, 2023

View reviewed changes

DeeDeeG merged commit edbb64c into master Jan 24, 2023

DeeDeeG mentioned this pull request Feb 7, 2023

deps: Update github to v0.36.14-pretranspiled pulsar-edit/pulsar#373

Merged

DeeDeeG mentioned this pull request Mar 4, 2023

rebrand git-tab-view #17

Merged

Spiker985 deleted the rebrand-getAtomAppName-fix-git-shell-out branch March 28, 2023 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Rebrand getAtomAppName() function (fix shelling out to `git` on macOS) #13

lib: Rebrand getAtomAppName() function (fix shelling out to `git` on macOS) #13

DeeDeeG commented Jan 17, 2023 •

edited

Loading

DeeDeeG commented Jan 17, 2023

savetheclocktower commented Jan 22, 2023

DeeDeeG commented Jan 23, 2023 •

edited

Loading

DeeDeeG commented Jan 23, 2023 •

edited

Loading

Spiker985 left a comment •

edited

Loading

confused-Techie left a comment

DeeDeeG commented Jan 24, 2023 •

edited

Loading

DeeDeeG commented Jan 24, 2023 •

edited

Loading

DeeDeeG commented Jan 24, 2023 •

edited

Loading

confused-Techie commented Jan 24, 2023

DeeDeeG commented Jan 24, 2023

DeeDeeG commented Jan 24, 2023

confused-Techie commented Jan 24, 2023

lib: Rebrand getAtomAppName() function (fix shelling out to git on macOS) #13

lib: Rebrand getAtomAppName() function (fix shelling out to git on macOS) #13

Conversation

DeeDeeG commented Jan 17, 2023 • edited Loading

DeeDeeG commented Jan 17, 2023

savetheclocktower commented Jan 22, 2023

DeeDeeG commented Jan 23, 2023 • edited Loading

DeeDeeG commented Jan 23, 2023 • edited Loading

Spiker985 left a comment • edited Loading

Choose a reason for hiding this comment

confused-Techie left a comment

Choose a reason for hiding this comment

DeeDeeG commented Jan 24, 2023 • edited Loading

DeeDeeG commented Jan 24, 2023 • edited Loading

DeeDeeG commented Jan 24, 2023 • edited Loading

confused-Techie commented Jan 24, 2023

DeeDeeG commented Jan 24, 2023

DeeDeeG commented Jan 24, 2023

confused-Techie commented Jan 24, 2023

lib: Rebrand getAtomAppName() function (fix shelling out to `git` on macOS) #13

lib: Rebrand getAtomAppName() function (fix shelling out to `git` on macOS) #13

DeeDeeG commented Jan 17, 2023 •

edited

Loading

DeeDeeG commented Jan 23, 2023 •

edited

Loading

DeeDeeG commented Jan 23, 2023 •

edited

Loading

Spiker985 left a comment •

edited

Loading

DeeDeeG commented Jan 24, 2023 •

edited

Loading

DeeDeeG commented Jan 24, 2023 •

edited

Loading

DeeDeeG commented Jan 24, 2023 •

edited

Loading