Refactor import hook API to isolate platform specific logic (#445)

etc/browser/lib/files.js

@@ -5,17 +5,17 @@
 function createError() { return new Error('unsupported in the browser'); }
 
 function createImportHook() {
-  return function (fpath, kind, cb) { cb(createError()); };
+  return function (_, cb) { cb(createError()); };
 }
 
 function createSyncImportHook() {
   return function () { throw createError(); };
 }
 
+function tryReadFileSync() { return null; }
 
 module.exports = {
   createImportHook,
   createSyncImportHook,
-  existsSync: function () { return false; },
-  readFileSync: function () { throw createError(); }
+  tryReadFileSync,
 };

lib/files.js

@@ -13,15 +13,18 @@ let fs = require('fs'),
 /** Default (asynchronous) file loading function for assembling IDLs. */
 function createImportHook() {
   let imports = {};
-  return function (fpath, kind, cb) {
-    fpath = path.resolve(fpath);
+  return function ({path: fpath, importerPath}, cb) {
+    fpath = path.resolve(path.dirname(importerPath), fpath);
     if (imports[fpath]) {
       // Already imported, return nothing to avoid duplicating attributes.
       process.nextTick(cb);
       return;
     }
     imports[fpath] = true;
-    fs.readFile(fpath, {encoding: 'utf8'}, cb);
+    fs.readFile(fpath, {encoding: 'utf8'}, (err, data) => {
+      if (err) return cb(err);
+      return cb(null, {contents: data, path: fpath});
+    });
   };
 }
 
@@ -35,22 +38,43 @@ function createImportHook() {
  */
 function createSyncImportHook() {
   let imports = {};
-  return function (fpath, kind, cb) {
-    fpath = path.resolve(fpath);
+  return function ({path: fpath, importerPath}, cb) {
+    fpath = path.resolve(path.dirname(importerPath), fpath);
     if (imports[fpath]) {
       cb();
     } else {
       imports[fpath] = true;
-      cb(null, fs.readFileSync(fpath, {encoding: 'utf8'}));
+      cb(null, {
+        contents: fs.readFileSync(fpath, {encoding: 'utf8'}),
+        path: fpath
+      });
     }
   };
 }
 
+/**
+ * Check if the given input string is "path-like" or a path to an existing file,
+ * and if so, read it. This requires it to contain a path separator
+ * (`path.sep`). If not, this will return `null`.
+ */
+function tryReadFileSync(str) {
+  if (
+    typeof str == 'string' &&
+    str.indexOf(path.sep) !== -1
+  ) {
+    try {
+      // Try interpreting `str` as path to a file.
+      return fs.readFileSync(str, {encoding: 'utf8'});
+    } catch (err) {
+      // If the file doesn't exist, return `null`. Rethrow all other errors.
+      if (err.code !== 'ENOENT') throw err;
+    }
+  }
+  return null;
+}
 
 module.exports = {
   createImportHook,
   createSyncImportHook,
-  // Proxy a few methods to better shim them for browserify.
-  existsSync: fs.existsSync,
-  readFileSync: fs.readFileSync
+  tryReadFileSync
 };

lib/specs.js

@@ -7,8 +7,7 @@
 /** IDL to protocol (services) and schema (types) parsing logic. */
 
 let files = require('./files'),
-    utils = require('./utils'),
-    path = require('path');
+    utils = require('./utils');
 
 
 // Default type references defined by Avro.
@@ -31,7 +30,7 @@ function assembleProtocol(fpath, opts, cb) {
     opts.importHook = files.createImportHook();
   }
 
-  importFile(fpath, (err, protocol) => {
+  importFile(fpath, '', (err, protocol) => {
     if (err) {
       cb(err);
       return;
@@ -57,19 +56,20 @@ function assembleProtocol(fpath, opts, cb) {
     cb(null, protocol);
   });
 
-  function importFile(fpath, cb) {
-    opts.importHook(fpath, 'idl', (err, str) => {
+  function importFile(fpath, importerPath, cb) {
+    opts.importHook({path: fpath, importerPath, kind: 'idl'}, (err, payload) => {
       if (err) {
         cb(err);
         return;
       }
-      if (str === undefined) {
+      if (!payload) {
         // This signals an already imported file by the default import hooks.
         // Implementors who wish to disallow duplicate imports should provide a
         // custom hook which throws an error when a duplicate is detected.
         cb();
         return;
       }
+      const {contents: str, path: fpath} = payload;
       let obj;
       try {
         let reader = new Reader(str, opts);
@@ -79,11 +79,11 @@ function assembleProtocol(fpath, opts, cb) {
         cb(err);
         return;
       }
-      fetchImports(obj.protocol, obj.imports, path.dirname(fpath), cb);
+      fetchImports(obj.protocol, obj.imports, fpath, cb);
     });
   }
 
-  function fetchImports(protocol, imports, dpath, cb) {
+  function fetchImports(protocol, imports, fpath, cb) {
     let importedProtocols = [];
     next();
 
@@ -104,9 +104,8 @@ function assembleProtocol(fpath, opts, cb) {
         cb(null, protocol);
         return;
       }
-      let importPath = path.join(dpath, info.name);
       if (info.kind === 'idl') {
-        importFile(importPath, (err, imported) => {
+        importFile(info.name, fpath, (err, imported) => {
           if (err) {
             cb(err);
             return;
@@ -118,24 +117,28 @@ function assembleProtocol(fpath, opts, cb) {
         });
       } else {
         // We are importing a protocol or schema file.
-        opts.importHook(importPath, info.kind, (err, str) => {
+        opts.importHook({
+          path: info.name,
+          importerPath: fpath,
+          kind: info.kind
+        }, (err, payload) => {
           if (err) {
             cb(err);
             return;
           }
           switch (info.kind) {
             case 'protocol':
             case 'schema': {
-              if (str === undefined) {
+              if (!payload) {
                 // Skip duplicate import (see related comment above).
                 next();
                 return;
               }
               let obj;
               try {
-                obj = JSON.parse(str);
+                obj = JSON.parse(payload.contents);
               } catch (err) {
-                err.path = importPath;
+                err.path = payload.path;
                 cb(err);
                 return;
               }
@@ -193,7 +196,7 @@ function assembleProtocol(fpath, opts, cb) {
  *
  * The parsing logic is as follows:
  *
- * + If `str` contains `path.sep` (on windows `\`, otherwise `/`) and is a path
+ * + If `str` contains `/` and is a path
  *   to an existing file, it will first be read as JSON, then as an IDL
  *   specification if JSON parsing failed. If either succeeds, the result is
  *   returned, otherwise the next steps are run using the file's content
@@ -207,15 +210,11 @@ function assembleProtocol(fpath, opts, cb) {
  */
 function read(str) {
   let schema;
-  if (
-    typeof str == 'string' &&
-    ~str.indexOf(path.sep) &&
-    files.existsSync(str)
-  ) {
-    // Try interpreting `str` as path to a file contain a JSON schema or an IDL
-    // protocol. Note that we add the second check to skip primitive references
-    // (e.g. `"int"`, the most common use-case for `avro.parse`).
-    let contents = files.readFileSync(str, {encoding: 'utf8'});
+  let contents = files.tryReadFileSync(str);
+
+  if (contents === null) {
+    schema = str;
+  } else {
     try {
       return JSON.parse(contents);
     } catch (err) {
@@ -224,8 +223,6 @@ function read(str) {
         schema = err ? contents : protocolSchema;
       });
     }
-  } else {
-    schema = str;
   }
   if (typeof schema != 'string' || schema === 'null') {
     // This last predicate is to allow `read('null')` to work similarly to

test/test_specs.js

@@ -449,9 +449,12 @@ suite('specs', () => {
     });
 
     test('import hook error', (done) => {
-      let hook = function (fpath, kind, cb) {
+      let hook = function ({path: fpath}, cb) {
         if (path.basename(fpath) === 'A.avdl') {
-          cb(null, 'import schema "hi"; protocol A {}');
+          cb(null, {
+            contents: 'import schema "hi"; protocol A {}',
+            path: fpath
+          });
         } else {
           cb(new Error('foo'));
         }
@@ -463,9 +466,12 @@ suite('specs', () => {
     });
 
     test('import hook idl error', (done) => {
-      let hook = function (fpath, kind, cb) {
+      let hook = function ({path: fpath}, cb) {
         if (path.basename(fpath) === 'A.avdl') {
-          cb(null, 'import idl "hi"; protocol A {}');
+          cb(null, {
+            contents: 'import idl "hi"; protocol A {}',
+            path: fpath
+          });
         } else {
           cb(new Error('bar'));
         }
@@ -624,11 +630,14 @@ suite('specs', () => {
 
     // Import hook from strings.
     function createImportHook(imports) {
-      return function (fpath, kind, cb) {
-        let key = path.normalize(fpath);
+      return function ({path: fpath, importerPath}, cb) {
+        let key = path.normalize(path.join(path.dirname(importerPath), fpath));
         let str = imports[key];
         delete imports[key];
-        process.nextTick(() => { cb(null, str); });
+        process.nextTick(() => { cb(null, typeof str === 'string' ? {
+          contents: str,
+          path: key
+        } : undefined); });
       };
     }
 

types/index.d.ts

@@ -127,8 +127,18 @@ interface IsValidOptions {
   noUndeclaredFields: boolean;
   errorHook: (path: string[], val: any, type: Type) => void
 }
+
+interface ImportHookPayload {
+  path: string;
+  type: 'idl' | 'protocol' | 'schema';
+}
+
+type ImportHookCallback = (err: any, params?: {contents: string, path: string}) => void;
+
+type ImportHook = (payload: ImportHookPayload, cb: ImportHookCallback) => void;
+
 interface AssembleOptions {
-  importHook: (filePath: string, type: 'idl', callback: Callback<object>) => void;
+  importHook: (params: ImportHookPayload, callback: Callback<object>) => void;
 }
 
 interface SchemaOptions {