User configurable cache folder for binary download

[nschonni]

New order of operations

1. Look for existing binary in vendor folder
2. Create target vendor folder
3. Look to see if we’ve cached a copy in the configured or NPM cache
4. Download to current version’s /CACHE/node-sass/version/binding_file
5. Copy the cached download to the regular vendor directory

Closes #1566

[nschonni]

@mriehema I think I figured out a version that will play nice with npm cache clean

[nschonni]

@xzyfer here is what I tried for manual testing

• Download/Install dev version when the version doesn't exist yet, then run a regular install overtop. OK
• Install multiple times. OK
• If you have a bad download, it just falls back to the build. OK
• Run npm cache clean node-sass@3.9.3, verify the folder is empty, re-run install. OK

Not sure what automated tests can be setup for this one though

[nschonni]

Arg, Node 0.10 support strikes again https://travis-ci.org/nschonni/node-sass/jobs/158644756#L308
Wonder if some of our other failures may be related, but swallowing the message

[mriehema]

@nschonni I will take a look the next days ASAP. Thank you for supporting my topic.

[xzyfer]

Using the npm cache was discussed (maybe offline, my bad). The problem is a lot of environments will clear the npm cache. This super common on CI and containers because the npm cache can cause weird side-effects between builds. So counter intuitively the places that would likely benefit the most from local binary caching wouldn't use it.

Caching the binaries in /tmp was nice because it isn't commonly clear, and it's easy for containerised/CI envs to opt-in to keeping it around without risking npm side effects.

My preference would be to have a location configurable using ENV vars like the rest of the binary download config. The npm config dir could be good default location, but I expect to really see much value you'd need to set it to something distinct from npm.

[xzyfer]

Wonder if some of our other failures may be related, but swallowing the message

No. This is a known limitation with cross-spawn. We just don't use sync. From their docs

If you are using spawnSync on node 0.10 or older, you will also need to install spawn-sync

[nschonni]

Caching the binaries in /tmp was nice because it isn't commonly clear, and it's easy for containerised/CI envs to opt-in to keeping it around without risking npm side effects.

Containers would lose the TMP the same why as the NPM cache. CI wise-its the same logic as we currently use

node-sass/appveyor.yml

Line 32 in 105cca2

- '%AppData%\npm-cache'

. The CI systems usually have their own settings now for keeping the project's node_modules, which means this won't hit either.

My preference would be to have a location configurable using ENV vars like the rest of the binary download config. The npm config dir could be good default location, but I expect to really see much value you'd need to set it to something distinct from npm.

That case already got covered by our other binary path options, this just adds resilincy for offline NPM installs without having an external setup.

---

That said, i'm not married to the approach, but I think this addresses the original concern in #1554 without adding more switches 😉

[nschonni]

No. This is a known limitation with cross-spawn. We just don't use sync. From their docs

Thansk, I'll change my spawn to the async version, and replace the fs.existsSync since I didn't realize it was depricated.

[xzyfer]

Containers would lose the TMP the same why as the NPM cache

Correct, but to my point, containerised/CI envs can easily to opt-in to keeping it around without risking npm side effects. Keeping the npm cache around is much more troublesome so you want these two decoupled.

That case already got covered by our other binary path options

These don't provide the fallback to vendor which is the big win here. I would expect this to fallback to SASS_BINARY_PATH instead of our vendor because that's the semantics of SASS_BINARY_PATH.

without adding more switches

Sane defaults are always better than switches, but giving users the ability to do what's best for their environment via switches when the defaults aren't sane for them is necessary.

[nschonni]

Sorry ignore the last push, it's not done yet. Forgot to set git config --global push.default current on this machine

[nschonni]

@xzyfer should be good to go now. The Appveyor failure seems like a weird unrelated npm issue where it can't find right version request for some reason.

• cleared out the spawn stuff by using the same process.env lookup for the npm cache directory.
• left in the fs.existsync for now since it is still used in a few places already.

[mriehema]

Hi, just checked out your branch and tested it on my Win7, 64bit machine. Works online, offline, with cleaned vendor directory, cleaned npm-cache.

Works perfectly and is so much faster. Thank you!
👍 🎉

[xzyfer]

nit: avoid negative conditional with an else branch. Inverting this if makes it much easier to read.

[nschonni]

Done

[xzyfer]

lgtm

[xzyfer]

Is it possible to get some test coverage over these changes? I'd hate to break installs in the future.

[nschonni]

I added a small test and slightly renamed the config key so it's less likely to be confused with the Ruby Sass cache stuff. I'll look at breaking the scripts in a separate PR.

[xzyfer]

lgtm