ppm: Install with the yarn install --production flag

[DeeDeeG]

Saves ~5-7MB disk space, depending on how you count it. Should make the packaged Pulsar binaries slightly smaller as well. Reduces file count, may marginally improve install times of Pulsar due to reduced disk I/O.

Description of the Change

Install the ppm dir with yarn install --production (omits devDependencies), rather than basic yarn install (which installs both dependencies AND devDependencies).

Quantitative Performance Benefits

Measuring by disk usage of the ppm dir, using ncdu.

• Before: Total disk usage: 213.6 MiB Apparent size: 168.0 MiB Items: 20613
• After: Total disk usage: 206.4 MiB Apparent size: 163.2 MiB Items: 19441

About 5-7 MB of disk savings (depending on how you measure it), 1172 fewer files/directories.

Possible Drawbacks

This omits the coffeescript and coffee-lint dependencies, among others. I don't think those (or anything in devDependencies are actually used in production. [ EDIT: actually, coffee-script is still there, since ppm's dependency season depends on it. ]

Risk seems minimal, but if we missed where these things are needed, it would cause bugs.

Note: apm and the Atom-community fork used to publish and install apm as a package from the npm package registry. This implicitly did not include any of the devDependencies as well. So I feel that proves out that the devdependencies are NOT needed in production, or at least they weren't needed as of then. I don't think we've mistakenly relied on a devDependency of ppm in production since those times.

Verification Process

Admittedly I haven't really tested this, but I can take some time to do so at some point.

Applicable Issues

None

Release Notes

Install ppm with the yarn install --production flag (save some disk space by excluding devDependencies)

[DeeDeeG]

Note that we could further save space and file count by omitting the spec/ dir (2.7 MB with 256 files/subdirs), script/ dir (8 files), native-module dir (5 files and a directory, this should probably be in spec/fixtures anyway)...

The above is roughly how much stuff got pruned out for upstream official Atom or for the atom-community fork.

[savetheclocktower]

Removing the coffeescript dependency seems risky to me. There are old packages that are written in CoffeeScript and we should make 100% sure that those will still work. I imagine some users also still have init.coffee files rather than init.js files.

[savetheclocktower]

(That said: if we turn out to need coffeescript for those reasons, then it should just be moved to dependencies. This PR is a good idea regardless.)

[DeeDeeG]

tl;dr: coffee-script isn't actually removed (whoops, I misspoke), and also this is just ppm's dependencies, not removing anything from under the main core repo's primary node_modules dir or anything like that.

@savetheclocktower thanks for raising those concerns.

I was a bit mistaken in my initial communication, besides probably being unclear what this PR was for.

First, for context, this is specifically optimizing how we install the core repo's copy of ppm. This PR doesn't touch the main dependencies of core repo, just the expected packages in ppm/node_modules when installing core's copy of ppm.

Also: it turns out I was mistaken about coffeescript actually being removed.

The diff from the first page of ncdu output looking at the top level of ppm's node_modules dir looks like this:

@@ -6,14 +6,12 @@
     2.0 MiB [          ] /es5-ext
     1.8 MiB [          ] /second-mate
     1.3 MiB [          ] /path-scurry
-    1.2 MiB [          ] /requirejs
     1.2 MiB [          ] /object.assign
     1.1 MiB [          ] /superagent
     1.1 MiB [          ] /ajv
     1.1 MiB [          ] /async
   940.0 KiB [          ] /escodegen
   824.0 KiB [          ] /make-fetch-happen
-  800.0 KiB [          ] /coffeelint
   732.0 KiB [          ] /ext
   728.0 KiB [          ] /libnpx
   708.0 KiB [          ] /bluebird
@@ -22,10 +20,8 @@
   584.0 KiB [          ] /uri-js
   556.0 KiB [          ] /request
   528.0 KiB [          ] /libcipm
-  524.0 KiB [          ] /coffeestack
   516.0 KiB [          ] /nan
   516.0 KiB [          ] /JSONStream
-  512.0 KiB [          ] /jasmine-node
   504.0 KiB [          ] /acorn
   500.0 KiB [          ] /get-uri
   492.0 KiB [          ] /vscode-oniguruma
@@ -35,14 +31,18 @@
   440.0 KiB [          ] /coffee-script
   432.0 KiB [          ] /foreground-child
   428.0 KiB [          ] /formidable
-  428.0 KiB [          ] /fileset
   424.0 KiB [          ] /resolve
   424.0 KiB [          ] /type
   416.0 KiB [          ] /node-addon-api
   396.0 KiB [          ] /iconv-lite
   396.0 KiB [          ] /@iarna
   380.0 KiB [          ] /es6-promise
-  364.0 KiB [          ] /express
   340.0 KiB [          ] /npm-lifecycle
   324.0 KiB [          ] /esprima
- Total disk usage: 132.9 MiB  Apparent size:  88.6 MiB  Items: 20201
+  308.0 KiB [          ] /tar
+  300.0 KiB [          ] /@isaacs
+  296.0 KiB [          ] /bl
+  292.0 KiB [          ] /sshpk
+  280.0 KiB [          ] /vm2
+  280.0 KiB [          ] /yargs
+ Total disk usage: 125.7 MiB  Apparent size:  83.9 MiB  Items: 19029

There are more packages than this, but this is the view I was looking at. (First page of a console-based disk usage analyzer's output.) I believe I mis-skimmed/misinterpreted the removal of coffeelint and coffeestack as being a removal of coffeescript itself.

You can see coffee-script (old package name with the hyphen) is still present/unchanged in the above diff. yarn why coffee-script shows the following...

% yarn why coffee-script
yarn why v1.22.19
[1/4] 🤔  Why do we have the module "coffee-script"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Has been hoisted to "coffee-script"
info Reasons this module exists
   - Specified in "devDependencies"
   - Hoisted from "jasmine-focused#jasmine-node#coffee-script"
   - Hoisted from "season#cson-parser#coffee-script"
info Disk size without dependencies: "452KB"
info Disk size with unique dependencies: "452KB"
info Disk size with transitive dependencies: "452KB"
info Number of shared dependencies: 0
=> Found "coffeelint#[email protected]"
info This module exists because "coffeelint" depends on it.
=> Found "coffeestack#[email protected]"
info This module exists because "jasmine-focused#jasmine-node#coffeestack" depends on it.
✨  Done in 1.07s.

Still there in --production mode, since season (the CSON parser library, I believe?) depends on it.

[ That said, I'm not clear if ppm needs coffee-script in production other than to read CSON files? Since it doesn't need to actually parse any CoffeeScript during run-time, as far as I know? It might copy some CoffeeScript files from the package templates, but without needing to understand/parse the files it's copying? But this bit is effectively mildly off-topic, since I don't think this question affects core repo, per se. ]

[savetheclocktower]

I was a bit mistaken in my initial communication, besides probably being unclear what this PR was for.

Nah, you indicated it was PPM in the title. I just didn't read closely enough.

[confused-Techie]

I think this PR is a fantastic idea, and the diff looks good to accomplish it! So here's a rubber stamp approval, although I'd really like us to see why one of our CI runs is failing here.

Looking into it myself I can't see anything that caused the failure.

It doesn't seem the module app-builder-lib has had any big recent changes that should affects it's structure.

[DeeDeeG]

I'd really like us to see why one of our CI runs is failing here.

Same, that's kind of making me scratch my head. A little nervous to merge this with CI failing for no clear reason as of yet.

I am also leaning toward holding off merging for now, honestly. Wanna sort that out.

[DeeDeeG]

The really strange thing is, this PR running yarn run build:apm, effectively cd ppm && yarn install --production...

And it's doing yarn install --production in the main dir instead of the ppm dir, thus breaking the build, because all the devDependencies of the main app are in fact missing.

So much for "not removing anything from under the main core repo's primary node_modules dir or anything like that."

This works for me on my local machine, not sure what about putting it in the CI workflow definition and doing it on a CI machine makes it not run in the correct directory.

[confused-Techie]

@DeeDeeG Just taking a look around, and studied up on this one.

Seems yarn may have a built in flag that can help us here.

yarn --cwd ./ppm install --production

[confused-Techie]

Just tested this one locally, and it does in fact seem to work as expected!

[DeeDeeG]

CI failed againwith the same issue, yarn is installing the main dir's dependencies in --production mode, so we miss needed devDependencies in main dir and I'm unclear of ppm ever gets installed.

Perhaps there is some weird env var set in CI that is interfering with how Yarn works, or some other odd aspect of the CI environment vs our development environments.