Add caching of node_modules and eslintcache to Github Actions

[mattrunyon]

Cache all node_modules folders. If any package-lock changes the cache will miss and trigger npm ci. Most of the time this should let us skip npm ci which also calls lerna bootstrap saving ~2min per run.

Also cache the .eslintcache files generated in each package. Currently those will rarely miss, if ever, but cache for each branch will be preferred. This should greatly speed up the eslint Jest test we have in each package. Locally npm run test:ci took ~54s w/o eslintcache and ~30s w/ eslintcache on a rerun without changing files.

[devinrsmith]

I'm not very knowledgeable about this repo or NPM, but shouldn't npm ci be a fast no-op if node_modules is already populated?

[mattrunyon]

It doesn't take too long (it actually only installs the dependencies of the root project), but the post install script which uses lerna to bootstrap and link the packages (and install their much larger list of dependencies) is slow (60-120s).

I know we can speed up lerna bootstrap by removing the hoist flag at the cost of space from redundant dependencies being installed in multiple packages. It will still take 45-60s though I think.

[devinrsmith]

So, using the presence of a cache hit to skip a full step is a bit hacky, and in general pretty dangerous. Is there any sort of lerna cache we could take advantage of? Is there a more appropriate task that we should be running, and we shouldn't need to do post install / lerna stuff?

[mattrunyon]

I agree it's a bit hacky. I don't know if it's necessarily that dangerous though since it's matching only if the hash of all package-locks is unchanged. The actual files can still change and be used correctly since lerna symlinks node_modules within the project (e.g. client-ui depends on grid. node_modules/@deephaven/grid is symlinked to packages/grid). If package-lock hasn't changed then that indicates the exact dependencies are the same as they were before.

Using yarn might fix part of the speed issue, but I was having other problems with yarn locally with the license files being symlinks (it couldn't find the file).

The vast majority of our PRs (90%+, probably higher) don't touch package-lock files. If we can speed up lerna + npm then this wouldn't be necessary, but npm is painfully slow when doing a no-op install.

Also I think npm ci actually deletes node_modules to start. Which it should only be deleting the small set of dependencies from the root. That on its own takes ~10 seconds for me locally when I remove the postinstall script.

[mofojed]

@devinrsmith why is using the cache hit to skip a step hacky? That's exactly what this action is for: https://github.com/actions/cache#skipping-steps-based-on-cache-hit

@mattrunyon There's no more sym linked license files so you should be good from there to try yarn again. I think npm ci should be clearing all the node_modules first, to make sure it has a false pass when a package is removed or something.

[mofojed]

I think we need to hoist, or we get two versions of react when building.

[mattrunyon]

Oh ya forgot that can cause dev issues. I was just testing it to see what the runtime was without it.

lerna/lerna#2605

Another solution someone had is to install react only at the root. Then you don't need hoist, but I also like hoist for reducing duplicate node modules.

It's a weird annoyance because when the components would be consumed outside the monorepo, these problems won't exist. I really wish lerna or yarn workspaces had a solution for not linking peer/dev deps in some way.

I also saw something about using a webpack alias but I don't think we can do that with CRA. I think the gist of it is in webpack you can tell it explicitly how to resolve react in the app build.

[mofojed]

@devinrsmith why is using the cache hit to skip a step hacky? That's exactly what this action is for: https://github.com/actions/cache#skipping-steps-based-on-cache-hit

@mattrunyon There's no more sym linked license files so you should be good from there to try yarn again. I think npm ci should be clearing all the node_modules first, to make sure it has a false pass when a package is removed or something.

[mofojed]

Use the Node - lerna example: https://github.com/actions/cache/blob/main/examples.md#node---lerna

[mattrunyon]

Effectively doing that, except since I'm skipping npm ci on cache hit I'm also hashing the main package.json file.

If we were to change the lerna flags on install/bootstrap and we weren't hashing that file, then it would be a cache hit and skip the install with different flags.

[mofojed]

Should include packages/*/package.json as well. In the case where someone accidentally updates a package.json but doesn't update the associated package-lock.json, we don't want to pull from the cache

If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.