fix: More robust importing.

My fix for #248 introduced
a stack on error objects that allowed errors to track the location of
the error within a series of imports. This turned out to be problematic
because our blocks are singletons and so the errors for that block also
want to be singletons. Because a block might be imported (and exported)
from different blocks, the stack needed to be tracked differently.

A previous commit added the CascadingError error for indicating an error
was caused by some previous error and used it for block exports. In this
commit, I rewrote the code to use CascadingError for block imports as
well.

This allowed me to stop doing weird things trying to parse a block a
second time if it previously had an error -- just to get a different
error instance.

Building on the MultipleBlockErrors, I've updated our error reporting in
a few places to describe the sequence of events that lead up to an
error.

The CLI now displays the root cause for an error like so:

```
error	test/fixtures/basic/error.block.css
	Two distinct classes cannot be selected on the same element: .foo.bar
	At test/fixtures/basic/error.block.css:1:5
	1: .foo.bar {
	2:   color: red;
caused	test/fixtures/basic/transitive-error.block.css
	Error in imported block.
	At test/fixtures/basic/transitive-error.block.css:1:1
	1: @block error from "./error.block.css";
	2:

error	test/fixtures/basic/transitive-error.block.css
	No Block named "error" found in scope.
	At test/fixtures/basic/transitive-error.block.css:4:3
	3: :scope {
	4:   extends: error;
	5: }

Found 2 errors in 1 file.
```

Author: chriseppstein

packages/@css-blocks/cli/src/index.ts

@@ -4,6 +4,7 @@ import {
 import { search as searchForConfiguration } from "@css-blocks/config";
 import {
   BlockFactory,
+  CascadingError,
   Configuration,
   CssBlockError,
   ErrorWithPosition,
@@ -203,16 +204,27 @@ export class CLI {
     this.exit(errorCount);
   }
 
+  fileForError(e: CssBlockError): string | undefined {
+    let loc = e.location;
+    if (!loc) return;
+    if (!errorHasRange(loc)) return;
+    if (!loc.filename) return;
+    return path.relative(process.cwd(), path.resolve(loc.filename));
+  }
+
   handleCssBlockError(blockFileRelative: string, error: CssBlockError): number {
-    if (error instanceof MultipleCssBlockErrors) {
+    if (error instanceof CascadingError) {
+      this.handleCssBlockError(this.fileForError(error.cause) || blockFileRelative, error.cause);
+    } else if (error instanceof MultipleCssBlockErrors) {
       let count = 0;
       for (let e of error.errors) {
-        count += this.handleCssBlockError(blockFileRelative, e);
+        count += this.handleCssBlockError(this.fileForError(e) || blockFileRelative, e);
+        this.println();
       }
       return count;
     }
     let loc = error.location;
-    let message = `${this.chalk.red("error")}\t${this.chalk.whiteBright(blockFileRelative)}`;
+    let message = `${this.chalk.red(error instanceof CascadingError ? "caused" : "error")}\t${this.chalk.whiteBright(this.fileForError(error) || blockFileRelative)}`;
     if (!errorHasRange(loc)) {
       this.println(message, error.origMessage);
     } else {
@@ -228,15 +240,6 @@ export class CLI {
     if (!errorHasRange(loc)) return;
     let filename = path.relative(process.cwd(), path.resolve(loc && loc.filename || blockFileRelative));
     this.println("\t" + this.chalk.bold.redBright(e.origMessage));
-    for (let referenceLocation of e.importStack.reverse()) {
-      if (referenceLocation.filename) {
-        referenceLocation.filename = path.relative(process.cwd(), path.resolve(referenceLocation.filename));
-      }
-      this.println(
-        this.chalk.bold.white("\tIn block referenced at"),
-        this.chalk.bold.whiteBright(charInFile(referenceLocation)),
-      );
-    }
     if (hasMappedPosition(loc)) {
       this.println(
         this.chalk.bold.white("\tAt compiled output of"),

packages/@css-blocks/cli/test/cli-test.ts

@@ -43,18 +43,26 @@ Found 1 error in 1 file.
     let cli = new CLI();
     await cli.run(["validate", fixture("basic/transitive-error.block.css")]);
 
-    assert.equal(cli.output,
-                 `error\t${relFixture("basic/transitive-error.block.css")}
+    assert.equal(
+      cli.output,
+      `error\ttest/fixtures/basic/error.block.css
 \tTwo distinct classes cannot be selected on the same element: .foo.bar
-\tAt ${relFixture("basic/error.block.css")}:1:5
+\tAt test/fixtures/basic/error.block.css:1:5
 \t1: .foo.bar {
 \t2:   color: red;
-error\t${relFixture("basic/transitive-error.block.css")}
+caused\ttest/fixtures/basic/transitive-error.block.css
+\tError in imported block.
+\tAt test/fixtures/basic/transitive-error.block.css:1:1
+\t1: @block error from "./error.block.css";
+\t2:
+
+error\ttest/fixtures/basic/transitive-error.block.css
 \tNo Block named "error" found in scope.
-\tAt ${relFixture("basic/transitive-error.block.css")}:4:3
+\tAt test/fixtures/basic/transitive-error.block.css:4:3
 \t3: :scope {
 \t4:   extends: error;
 \t5: }
+
 Found 2 errors in 1 file.
 `);
     assert.equal(cli.exitCode, 2);

packages/@css-blocks/core/src/BlockParser/BlockFactory.ts

@@ -125,7 +125,9 @@ export class BlockFactory {
   }
 
   getBlock(identifier: FileIdentifier): Promise<Block> {
-    if (this.promises[identifier]) {
+    if (this.blocks[identifier]) {
+      return Promise.resolve(this.blocks[identifier]);
+    } else if (this.promises[identifier]) {
       return this.promises[identifier].catch(() => {
         // If we got an error last time, try again.
         // Also this makes sure that the error object gives a correct import stack trace.

packages/@css-blocks/core/src/BlockParser/features/import-blocks.ts

@@ -5,12 +5,17 @@ import { BLOCK_IMPORT, CLASS_NAME_IDENT, DEFAULT_EXPORT, isBlockNameReserved } f
 import { Block } from "../../BlockTree";
 import * as errors from "../../errors";
 import { sourceRange } from "../../SourceLocation";
-import { allDone } from "../../util";
 import { BlockFactory } from "../index";
 import { parseBlockNames, stripQuotes } from "../utils";
 
 const FROM_EXPR = /\s+from\s+/;
 
+interface ParsedImport {
+  blockPath: string;
+  atRule: postcss.AtRule;
+  names: Array<{localName: string; remoteName: string}>;
+}
+
 /**
  * Resolve all block references for a given block.
  * @param block Block to resolve references for
@@ -19,102 +24,125 @@ const FROM_EXPR = /\s+from\s+/;
 export async function importBlocks(block: Block, factory: BlockFactory, file: string): Promise<Block> {
 
   let root: postcss.Root | undefined = block.stylesheet;
-  let namedBlockReferences: Promise<[string, string, postcss.AtRule, Block]>[] = [];
 
   if (!root) {
-    block.addError(new errors.InvalidBlockSyntax(`Error finding PostCSS root for block ${block.name}`));
-  } else {
-    // For each `@block` expression, read in the block file, parse and
-    // push to block references Promise array.
-    root.walkAtRules(BLOCK_IMPORT, (atRule: postcss.AtRule) => {
-      // imports: `<blocks-list> from <block-path>`
-      // blockList: `<default-block> | <named-blocks> | <default-block> " , " <named-blocks> | <named-blocks> " , " <default-block>`
-      // blockPath: `' " ' <any-value> ' " ' | " ' " <any-value> " ' "`
-      let imports = atRule.params;
-      let [blockList = "", blockPath = ""] = imports.split(FROM_EXPR);
-      blockPath = stripQuotes(blockPath);
-
-      if (!blockList || !blockPath) {
-        block.addError(new errors.InvalidBlockSyntax(
-          `Malformed block reference: \`@block ${atRule.params}\``,
-          sourceRange(factory.configuration, block.stylesheet, file, atRule),
-          ));
-      } else {
-        // Import file, then parse file, then save block reference.
-        let blockPromise: Promise<Block> = factory.getBlockRelative(block.identifier, blockPath);
-
-        blockPromise = blockPromise.catch((e) => {
-          if (e instanceof errors.CssBlockError) {
-            e.importStack.push(sourceRange(factory.configuration, block.stylesheet, file, atRule));
-          }
-          throw e;
-        });
+    block.addError(new errors.InvalidBlockSyntax(`Internal Error: Cannot find PostCSS root for block ${block.name}`));
+    return block;
+  }
 
-        let blockNames = parseBlockNames(blockList, true);
-        for (let localName of Object.keys(blockNames)) {
-          let remoteName = blockNames[localName];
-          // Validate our imported block name is a valid CSS identifier.
-          if (!CLASS_NAME_IDENT.test(localName)) {
-            block.addError(new errors.InvalidBlockSyntax(
-              `Illegal block name in import. "${localName}" is not a legal CSS identifier.`,
-              sourceRange(factory.configuration, block.stylesheet, file, atRule),
-            ));
-          }
-          if (!CLASS_NAME_IDENT.test(remoteName)) {
-            block.addError(new errors.InvalidBlockSyntax(
-              `Illegal block name in import. "${remoteName}" is not a legal CSS identifier.`,
-              sourceRange(factory.configuration, block.stylesheet, file, atRule),
-            ));
-          }
-          if (localName === DEFAULT_EXPORT && remoteName === DEFAULT_EXPORT) {
-            block.addError(new errors.InvalidBlockSyntax(
-              `Default Block from "${blockPath}" must be aliased to a unique local identifier.`,
-              sourceRange(factory.configuration, block.stylesheet, file, atRule),
-            ));
-          }
-          if (isBlockNameReserved(localName)) {
-            block.addError(new errors.InvalidBlockSyntax(
-              `Cannot import "${remoteName}" as reserved word "${localName}"`,
-              sourceRange(factory.configuration, block.stylesheet, file, atRule),
-            ));
-          }
+  let parsedImports = new Array<ParsedImport>();
+  // For each `@block` expression, read in the block file, parse and
+  // push to block references Promise array.
+  root.walkAtRules(BLOCK_IMPORT, (atRule: postcss.AtRule) => {
+    // imports: `<blocks-list> from <block-path>`
+    // blockList: `<default-block> | <named-blocks> | <default-block> " , " <named-blocks> | <named-blocks> " , " <default-block>`
+    // blockPath: `' " ' <any-value> ' " ' | " ' " <any-value> " ' "`
+    let imports = atRule.params;
+    let [blockList = "", blockPath = ""] = imports.split(FROM_EXPR);
+    blockPath = stripQuotes(blockPath);
 
-          // Once block is parsed, save named block reference
-          let namedResult: Promise<[string, string, postcss.AtRule, Block]> = blockPromise.then((block: Block): [string, string, postcss.AtRule, Block] => {
-            let referencedBlock = block.getExportedBlock(remoteName);
-            if (!referencedBlock) {
-              throw new errors.InvalidBlockSyntax(
-                `Cannot import Block "${remoteName}". No Block named "${remoteName}" exported by "${blockPath}".`,
-                sourceRange(factory.configuration, block.stylesheet, file, atRule),
-              );
-            }
-            return [localName, blockPath, atRule, referencedBlock];
-          });
-          namedBlockReferences.push(namedResult);
+    if (!blockList || !blockPath) {
+      block.addError(new errors.InvalidBlockSyntax(
+        `Malformed block reference: \`@block ${atRule.params}\``,
+        sourceRange(factory.configuration, block.stylesheet, file, atRule),
+        ));
+    } else {
+      let names: ParsedImport["names"] = [];
+      let blockNames = parseBlockNames(blockList, true);
+      for (let localName of Object.keys(blockNames)) {
+        let remoteName = blockNames[localName];
+        let hasInvalidNames = validateBlockNames(factory.configuration, block, blockPath, localName, remoteName, file, atRule);
+        if (!hasInvalidNames) {
+          names.push({ localName, remoteName });
         }
       }
-    });
+      parsedImports.push({ blockPath, atRule, names });
+    }
+  });
+
+  let localNames: ObjectDictionary<string> = {};
+  for (let parsedImport of parsedImports) {
+    let {blockPath, atRule, names} = parsedImport;
+    let referencedBlock: Block | null = null;
 
-    // When all import promises have resolved, save the block references and
-    // resolve.
-    let results: Array<[string, string, postcss.AtRule, Block]>;
+    // Import the main block file referenced by the import path.
     try {
-      results = await allDone(namedBlockReferences);
-      let localNames: ObjectDictionary<string> = {};
-      results.forEach(([localName, importPath, atRule, otherBlock]) => {
-        if (localNames[localName]) {
+      referencedBlock = await factory.getBlockRelative(block.identifier, parsedImport.blockPath);
+    } catch (err) {
+      block.addError(new errors.CascadingError(
+        "Error in imported block.",
+        err,
+        sourceRange(factory.configuration, block.stylesheet, file, atRule),
+      ));
+    }
+
+    for (let {localName, remoteName} of names) {
+      // check for duplicate local names
+      if (localNames[localName]) {
+        block.addError(new errors.InvalidBlockSyntax(
+          `Blocks ${localNames[localName]} and ${blockPath} cannot both have the name ${localName} in this scope.`,
+          sourceRange(factory.configuration, block.stylesheet, file, atRule),
+        ));
+        continue;
+      } else {
+        localNames[localName] = blockPath;
+      }
+
+      // Store a reference to the local block if possible
+      if (referencedBlock) {
+        let exportedBlock = referencedBlock.getExportedBlock(remoteName);
+        if (exportedBlock) {
+          block.addBlockReference(localName, exportedBlock);
+        } else {
           block.addError(new errors.InvalidBlockSyntax(
-            `Blocks ${localNames[localName]} and ${importPath} cannot both have the name ${localName} in this scope.`,
+            `Cannot import Block "${remoteName}". No Block named "${remoteName}" exported by "${blockPath}".`,
             sourceRange(factory.configuration, block.stylesheet, file, atRule),
           ));
-        } else {
-          block.addBlockReference(localName, otherBlock);
-          localNames[localName] = importPath;
         }
-      });
-    } catch (e) {
-      block.addError(e);
+      }
     }
   }
   return block;
 }
+
+function validateBlockNames(
+  config: BlockFactory["configuration"],
+  block: Block,
+  blockPath: string,
+  localName: string,
+  remoteName: string,
+  file: string,
+  atRule: postcss.AtRule,
+): boolean {
+  let hasInvalidNames = false;
+  // Validate our imported block name is a valid CSS identifier.
+  if (!CLASS_NAME_IDENT.test(localName)) {
+    hasInvalidNames = true;
+    block.addError(new errors.InvalidBlockSyntax(
+      `Illegal block name in import. "${localName}" is not a legal CSS identifier.`,
+      sourceRange(config, block.stylesheet, file, atRule),
+    ));
+  }
+  if (!CLASS_NAME_IDENT.test(remoteName)) {
+    hasInvalidNames = true;
+    block.addError(new errors.InvalidBlockSyntax(
+      `Illegal block name in import. "${remoteName}" is not a legal CSS identifier.`,
+      sourceRange(config, block.stylesheet, file, atRule),
+    ));
+  }
+  if (localName === DEFAULT_EXPORT && remoteName === DEFAULT_EXPORT) {
+    hasInvalidNames = true;
+    block.addError(new errors.InvalidBlockSyntax(
+      `Default Block from "${blockPath}" must be aliased to a unique local identifier.`,
+      sourceRange(config, block.stylesheet, file, atRule),
+    ));
+  }
+  if (isBlockNameReserved(localName)) {
+    hasInvalidNames = true;
+    block.addError(new errors.InvalidBlockSyntax(
+      `Cannot import "${remoteName}" as reserved word "${localName}"`,
+      sourceRange(config, block.stylesheet, file, atRule),
+    ));
+  }
+  return hasInvalidNames;
+}

packages/@css-blocks/core/src/errors.ts

@@ -26,13 +26,11 @@ interface HasPrefix {
 export class CssBlockError extends Error {
   static prefix = "Error";
   origMessage: string;
-  importStack: Array<SourceLocation.SourceRange | SourceLocation.MappedSourceRange | SourceLocation.SourceFile>;
   private _location?: ErrorLocation;
   constructor(message: string, location?: ErrorLocation) {
     super(message);
     this.origMessage = message;
     this._location = location;
-    this.importStack = new Array();
     super.message = this.annotatedMessage();
   }
 
@@ -133,7 +131,7 @@ export class BlockPathError extends CssBlockError {
  * clear errors
  */
 export class MultipleCssBlockErrors extends CssBlockError {
-  static prefix = "MultipleCssBlockErrors";
+  static prefix = "Caused by multiple errors";
   private _errors: CssBlockError[] = [];
 
   constructor(errors: CssBlockError[], location?: ErrorLocation, details?: string) {
@@ -151,10 +149,7 @@ export class MultipleCssBlockErrors extends CssBlockError {
     }
     if (!details) {
       details = ":";
-      let i = 0;
-      for (let err of this._errors) {
-        details += `\n\t${++i}. ${err}`;
-      }
+      details += errorDetails(this);
     }
     this.message += details;
   }
@@ -178,3 +173,22 @@ export class CascadingError extends CssBlockError {
     this.cause = rootCause;
   }
 }
+
+function errorDetails(error: MultipleCssBlockErrors, indent = ""): string {
+  let details = "";
+  let i = 0;
+  for (let err of error.errors) {
+    details += `\n${indent}${++i}. ${err}`;
+    if (err instanceof CascadingError) {
+      if (err.cause instanceof MultipleCssBlockErrors && err.cause.errors.length > 1) {
+        details += `\n${indent}   ${i > 9 ? " " : ""}Caused by multiple errors:`;
+        details += errorDetails(err.cause, indent + "\t");
+      } else {
+        let cause = err.cause instanceof MultipleCssBlockErrors ? err.cause.errors[0] : err.cause;
+        details += `\n${indent}    ${i > 9 ? " " : ""}Caused by:`;
+        details += `\n${indent} ${cause}`;
+      }
+    }
+  }
+  return details;
+}