Made sure storm-replay is available before we try to use it. #22
Conversation
index.js
Outdated
| @@ -115,7 +115,7 @@ const openArchive = function (file, noCache) { | |||
| // ensure non-breaking changes | |||
| exports.get = (file, archive) => { | |||
| log.debug('get() : ' + file + ', ' + archive); | |||
| if (['darwin', 'linux'].indexOf(process.platform) > -1) { | |||
| if (storm && ['darwin', 'linux'].indexOf(process.platform) > -1) { | |||
There was a problem hiding this comment.
You can go ahead and remove the OS check. Past me was being lazy.
There was a problem hiding this comment.
I did not test storm-extract on Windows, so I automatically excluded it.
Your method is "more correct", which permits non-storm-extract on OSX/Linux (why someone would want to wait? No clue.)
There was a problem hiding this comment.
I tried to get storm-replay to build, I wasn't able to in Node 12.6.3. So it wasn't an option for me, unless I just missed something in the build setup.
I'll update that check tonight when I get a few minutes. Thanks for taking a look at it so quickly.
There was a problem hiding this comment.
As stated in issue #21, storm-replay will not build in node12 due to deprecated calls. It will need to be updated. As of right now, storm-replay builds in node v10 and node-gyp v6.1.0.
storm-replay DOES have an issue with node-gyp v7.
This makes sure we don't try to use storm-replay if it's not available. I believe this fixes a problem where a parse on MacOS or Linux would fail if the storm-replay dependency has not been installed.
You could probably drop the OS check and just check for storm, but I don't know what the expectations are on Windows, so I left it alone.