Fix dugite

[confused-Techie]

EDIT:

As helpful pointed out by @DeeDeeG we are using yarn v1.x.x and the .yarnrc.yml file isn't actually making any changes. So in all reality, yarn will be just as stable as before, and I'll cross out where this PR mentions the possibility of anything different.

---

TLDR:

This PR fixes the below errors that would appear in the console, as reported by users on Discord, and seen by me. While also aiming to improve our yarn install stability, try and lock down what's included in our app.asar and unpacked from the asar archive. While additionally increasing our final install size a little bit more.

Issue 1

Uncaught (in promise) Error: Git could not be found at the expected path: 'C:\Users<user>\AppData\Local\Programs\pulsar\resources\app.asar.unpacked\node_modules\dugite\git\cmd\git.exe'. This might be a problem with how the application is packaged, so confirm this folder hasn't been removed when packaging.
    at C:\Users\<user>\AppData\Local\Programs\pulsar\resources\app.asar\node_modules\dugite\build\lib\git-process.js:87:39
    at exithandler (child_process.js:324:5)
    at ChildProcess.errorhandler (child_process.js:336:5)
    at ChildProcess.emit (events.js:315:20)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:275:12)
    at onErrorNT (internal/child_process.js:465:16)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)

Issue 2

Uncaught (in promise) Error: ERR_FILE_NOT_FOUND (-6) loading 'file:///C:/Users/<user>/AppData/Local/Programs/pulsar/resources/app.asar.unpacked/node_modules/github/lib/renderer.html?js=C%3A%5CUsers%5Cantho%5CAppData%5CLocal%5CPrograms%5Cpulsar%5Cresources%5Capp.asar.unpacked%5Cnode_modules%5Cgithub%5Clib%5Cworker.js&managerWebContentsId=1&operationCountLimit=10&channelName=github%3Arenderer-ipc'
    at rejectAndCleanup (electron/js2c/browser_init.js:217:1457)
    at Object.failListener (electron/js2c/browser_init.js:217:1670)
    at Object.emit (events.js:315:20)

---

So mainly this PR was aimed at fixing the above issues, but while doing so ended up taking a bit of a deeper dive into our build process than originally intended. Where I wanted to try and make as many optimizations as possible, and try to increase our installation stability.

The main fix for the above error, was firstly running the Windows binary installation while using scripts, since dugite uses a postInstall script to install it's bundled git, and without scripts this wasn't happening, which would cause the file to not appear in our asar. Additionally though we needed to unpack this file from our asar to allow it to be imported. The second error shown was also fixed by adding more items to be unpacked.

But beyond these immediate fixes, I'll detail the other changes made.

Otherwise essentially this PR modifies what's included in our asar bundle, and what's unpacked from the asar archive. While again trying to increase the yarn install stability. Since as it is currently the CI has it written to try installing multiple times.

For what is now unpacked

• "node_modules/github/lib/*" - Resolves Issue 2
• "/node_modules/dugite/git/" - Resolves Issue 1
• "/node_modules/spellchecker/" - Matches more of what Atom originally unpacked, in the hopes of avoiding an issue in the future.

Attempting to Help yarn install

I've added a .yarnrc.yml file to the root of the project.

This file has been removed due to incompatibility with yarn v1.x.x as helpfully pointed out.

In it I've set two values:

nodeLinker: "node-modules"
pnpMode: "loose"

The first is recommended by yarn when migrating from NPM to Yarn, the second value pnpMode seemed to help ensure some of the modules we are unpacking were available in my environment. And seems to more closely mirror the behavior of npm which obviously we can assume many of our packages will expect. More details about what this changes in how our dependencies are linked is available on the yarn website.

Excludes

From here I went through and started adding some values to what we exclude from our asar archive. Since after running the scripts and including their output into our asar bundle, once installed Pulsar (at least on Windows) went from ~900MB to 1.10GB. Which obviously is not ideal. But after adding these additional excludes I was able to cut down that size to 1.05GB and likely with more optimization on what we exclude we can decrease the size further.

So taking a look at what Atom had originally thought to exclude I mirrored many of those decisions back into our exclusions. Including a couple of things that where previously set to include. Namely things like our main repos docs and scripts.

So feel free to review and provide feedback if this isn't a change we want, especially for things like the .yarnrc.yml.(This file has been removed from this PR)
I just thought this was a method we could try to prevent additional issues from popping up. But really if we wanted to prioritize only solving the issue at hand, we could look at allowing dugite scripts to run by providing some dependenciesMeta or something along those lines.

But thanks for any review if anyone's able to take a look at this.

---

Beyond that while doing all of this research I found some other interesting things which I wanted to detail here for posterity or future reference.

Atom Asar Archive

• benchmarks/
• dot-atom/
• exports/
• less-compile-cache/
• node_modules/
• resources/
• spec/
• src/
• static/
• vendor/
• package.json

Pulsar Asar Archive (Before PR)

• docs/
• dot-atom/
• exports/
• keymaps/
• menus/
• node_modules/
• resources/
• scipt/
• spec/
• src/
• static/
• vendor/
• package.json

Atom Unpacked Asar

node_modules/

• @atom
• ctags
• dugite
• fs-admin
• fswin
• github
• git-utils
• keyboard-layout
• keytar
• nslog
• oniguruma
• pathwatcher
• scrollbar-style
• spellchecker
• superstring
• symbols-view
• text-buffer
• tree-sitter
• tree-sitter-bash
• tree-sitter-c
• tree-sitter-cpp
• tree-sitter-css
• tree-sitter-embeded-template
• tree-sitter-go
• tree-sitter-html
• tree-sitter-java-dev
• tree-sitter-javascript
• tree-sitter-jsdoc
• tree-sitter-json
• tree-sitter-python
• tree-sitter-regex
• tree-sitter-rub
• tree-sitter-rust
• tree-sitter-typescript
• vscode-ripgrep

Pulsar Unpacked Asar (After PR)

node_modules/

• dugite
• github
• spellchecker
• symbols-view
• vscode-ripgrep
• whats-my-line

[DeeDeeG]

Taking a quick look at this, by the way if I understand correctly we should be using Yarn 1.x

Yarn had a complete massive 2.x re-write, and pretty much everything about Yarn 2.x changed. 1.x is left essentially as-is. ("its stability (and lack of churn) is a gift to the community", to paraphrase the main maintainer/developer of Yarn.)

So yeah, Yarn 1.x "classic" and 2.x+ "berry" exist in parallel and are almost totally different things.

Those docs and config settings are probably specific to Yarn 2+?

See: https://classic.yarnpkg.com VS https://yarnpkg.com/

[confused-Techie]

@DeeDeeG fantastic catch, I'm super new to yarn and didn't realize the difference. So thanks for pointing it out, and indeed you are right that the .yarnrc.yml doesn't seem to be needed or in this instance doing anything. Must've made other concurrent changes that resolved my issues and attributed them to the config. Thanks for that, I'll remove them from this PR

[DeeDeeG]

Thanks for all the research, the writeup, the bug fixes, big 👍 from me!

I tried to look for anything that might have seemed amiss (or that maybe you had missed, rhyme not intended) and came up with nothing. It all checked out more and made more sense the more I looked into it.

Looked back at some Atom PRs where these files had churn (especially atom/atom#14682, but also atom/atom#12410) and it only left me feeling more confident that we were simply running into issues upstream Atom had already run into and solved similarly to what we are doing here. 👍 👍 👍 👍 👍

(If I was confused about some of the file inclusions/exclusions/unpacks before, this made me less so.)

[Edit to add: I think there may be an opportunity for more aggressive file exclusions from the bundled app, but this is a good start! And a great reminder to potentially look into that.]

I was already going to give an approval based on it working in testing and fixing some real issues, without causing any noticeable problems in testing. But I felt bad giving such cursory reviews, so I decided to look harder and do my research, now I'm feeling quite good about this one.

Beyond that while doing all of this research I found some other interesting things which I wanted to detail here for posterity or future reference.

👀 I am finding this very interesting, maybe something for folks to come back to when we get some time. I wonder why Atom unpacks all those language-treesitter packages... Hmm.

Thanks for finally doing this, I feel a bit bad bringing it up multiple times and not doing it myself, yet here we are. Thanks! Another two bugs sorted, if not more. ✨ 👍

[DeeDeeG]

Extra credit question, not at all a blocker for merging this IMO: what did you learn about the Yarn install? 👀

echo "There is a reason for so many tries"

(I am guessing it's just "this doesn't work sometimes"...? But if you learned anything more, I'd be curious.)

[confused-Techie]

To be honest this comment was more a clarification. Since the original install_without_scripts_script has 4 total attempts to install, and at the end says "Giving Up", my message here is more saying if we ever see that, then there was a reason we originally had 4 retries, rather than just one. But as of currently in my testing, I never encountered a single failure. Which really then what I've learned is that seemingly our install process (at least on Windows) is much more stable than when these scripts where originally made. Since we likely could get away with having the single npx yarn install

[confused-Techie]

Thanks for the approval @DeeDeeG, and no worries, we all have to many threads going on here.

But I didn't even originally find both of those so super interesting reads. And yeah after getting this merged it's on my todo list to take another good look at this, and do two things, organize our exclusions and additionally add as many as I possibly can. While a cursory search of the packed ASAR doesn't show to many things there that we don't need, obviously we would rather have absolutely nothing that we don't properly need.

But again thanks, I'll leave this up for a short while longer to ensure everyone else on @pulsar-edit/core has a chance to look at it, then will merge