Warn instead of error when an fs call cannot be evaluated (#587)

Author: fathyb

src/Logger.js

@@ -44,12 +44,16 @@ class Logger {
     this.write(this.chalk.bold(message), true);
   }
 
-  warn(message) {
+  warn(err) {
     if (this.logLevel < 2) {
       return;
     }
 
+    let {message, stack} = prettyError(err, {color: this.color});
     this.write(this.chalk.yellow(`${emoji.warning}  ${message}`));
+    if (stack) {
+      this.write(stack);
+    }
   }
 
   error(err) {

src/WorkerFarm.js

@@ -39,6 +39,7 @@ class WorkerFarm extends Farm {
 
   async initRemoteWorkers(options) {
     this.started = false;
+    this.warmWorkers = 0;
 
     let promises = [];
     for (let i = 0; i < this.options.maxConcurrentWorkers; i++) {
@@ -52,36 +53,40 @@ class WorkerFarm extends Farm {
   }
 
   receive(data) {
-    if (!this.children[data.child]) {
-      // This handles premature death
-      // normally only accurs for workers
-      // that are still warming up when killed
-      return;
-    }
-
     if (data.event) {
       this.emit(data.event, ...data.args);
     } else if (data.type === 'logger') {
-      logger.handleMessage(data);
-    } else {
+      if (this.shouldUseRemoteWorkers()) {
+        logger.handleMessage(data);
+      }
+    } else if (this.children[data.child]) {
       super.receive(data);
     }
   }
 
+  shouldUseRemoteWorkers() {
+    return this.started && this.warmWorkers >= this.activeChildren;
+  }
+
   async run(...args) {
     // Child process workers are slow to start (~600ms).
     // While we're waiting, just run on the main thread.
     // This significantly speeds up startup time.
-    if (this.started && this.warmWorkers >= this.activeChildren) {
+    if (this.shouldUseRemoteWorkers()) {
       return this.remoteWorker.run(...args, false);
     } else {
       // Workers have started, but are not warmed up yet.
       // Send the job to a remote worker in the background,
       // but use the result from the local worker - it will be faster.
       if (this.started) {
-        this.remoteWorker.run(...args, true).then(() => {
-          this.warmWorkers++;
-        });
+        this.remoteWorker.run(...args, true).then(
+          () => {
+            this.warmWorkers++;
+          },
+          () => {
+            // ignore error
+          }
+        );
       }
 
       return this.localWorker.run(...args, false);

src/visitors/fs.js

@@ -2,6 +2,7 @@ const t = require('babel-types');
 const Path = require('path');
 const fs = require('fs');
 const template = require('babel-template');
+const logger = require('../Logger');
 
 const bufferTemplate = template('Buffer(CONTENT, ENC)');
 
@@ -31,12 +32,32 @@ module.exports = {
         __dirname: Path.dirname(asset.name),
         __filename: asset.basename
       };
-      let [filename, ...args] = path
-        .get('arguments')
-        .map(arg => evaluate(arg, vars));
-      filename = Path.resolve(filename);
+      let filename, args, res;
+
+      try {
+        [filename, ...args] = path
+          .get('arguments')
+          .map(arg => evaluate(arg, vars));
+
+        filename = Path.resolve(filename);
+        res = fs.readFileSync(filename, ...args);
+      } catch (err) {
+        if (err instanceof NodeNotEvaluatedError) {
+          // Warn using a code frame
+          err.fileName = asset.name;
+          asset.generateErrorMessage(err);
+          logger.warn(err);
+          return;
+        }
+
+        // Add location info so we log a code frame with the error
+        err.loc =
+          path.node.arguments.length > 0
+            ? path.node.arguments[0].loc.start
+            : path.node.loc.start;
+        throw err;
+      }
 
-      let res = fs.readFileSync(filename, ...args);
       let replacementNode;
       if (Buffer.isBuffer(res)) {
         replacementNode = bufferTemplate({
@@ -153,6 +174,12 @@ function getBindingPath(path, name) {
   return binding && binding.path;
 }
 
+function NodeNotEvaluatedError(node) {
+  this.message = 'Cannot statically evaluate fs argument';
+  this.node = node;
+  this.loc = node.loc.start;
+}
+
 function evaluate(path, vars) {
   // Inline variables
   path.traverse({
@@ -165,8 +192,9 @@ function evaluate(path, vars) {
   });
 
   let res = path.evaluate();
+
   if (!res.confident) {
-    throw new Error('Could not statically evaluate fs call');
+    throw new NodeNotEvaluatedError(path.node);
   }
 
   return res.value;

test/fs.js

@@ -76,6 +76,41 @@ describe('fs', function() {
       assert.equal(typeof output.test, 'function');
       assert.equal(output.test(), 'test-pkg-ignore-fs-ok');
     });
+
+    // TODO: check if the logger has warned the user
+    it('should ignore fs calls when the filename is not evaluable', async function() {
+      let b = await bundle(
+        __dirname + '/integration/fs-file-non-evaluable/index.js'
+      );
+      let thrown = false;
+
+      try {
+        run(b);
+      } catch (e) {
+        assert.equal(e.message, 'require(...).readFileSync is not a function');
+
+        thrown = true;
+      }
+
+      assert.equal(thrown, true);
+    });
+
+    it('should ignore fs calls when the options are not evaluable', async function() {
+      let b = await bundle(
+        __dirname + '/integration/fs-options-non-evaluable/index.js'
+      );
+      let thrown = false;
+
+      try {
+        run(b);
+      } catch (e) {
+        assert.equal(e.message, 'require(...).readFileSync is not a function');
+
+        thrown = true;
+      }
+
+      assert.equal(thrown, true);
+    });
   });
 
   describe('--target=node', function() {

test/integration/fs-file-non-evaluable/index.js

@@ -0,0 +1 @@
+module.exports = require('fs').readFileSync(Date.now())

test/integration/fs-options-non-evaluable/index.js

@@ -0,0 +1,5 @@
+const dir = __dirname
+
+module.exports = require('fs').readFileSync(dir + '/test.txt', {
+  encoding: (typeof Date.now()).replace(/number/, 'utf-8')
+})