Merge pull request #188 from typed-ember/fail-the-build-but-on-purpose

Fail the build when `noEmitOnError` is set and there's a type error

Author: chriskrycho

README.md

@@ -76,7 +76,7 @@ Support for (mostly in the form of documentation!) for the changes in Ember 3.1
 
 ### `tsconfig.json`
 
-We generate a good default [`tsconfig.json`][blueprint], which will usually make everything _Just Work_. In general, you may customize your TypeScript build process as usual using the `tsconfig.json` file.
+We generate a good default [`tsconfig.json`][blueprint], which will usually make everything _Just Work™_. In general, you may customize your TypeScript build process as usual using the `tsconfig.json` file.
 
 However, there are a few things worth noting if you're already familiar with TypeScript and looking to make further or more advanced customizations (i.e. _most_ users can just ignore this section!):
 
@@ -116,13 +116,31 @@ In addition to the points made below, you may find the [Typing Your Ember][typin
 
 ### Incremental adoption
 
-If you are porting an existing app to TypeScript, you can install this addon and migrate your files incrementally by changing their extensions from `.js` to `.ts`. As TypeScript starts to find errors (and it usually does!), make sure to celebrate your (small) wins with your team, especially if some people are not convinced yet. We would also love to hear your stories!
+If you are porting an existing app to TypeScript, you can install this addon and migrate your files incrementally by changing their extensions from `.js` to `.ts`. As TypeScript starts to find errors (and it usually does!), make sure to celebrate your wins – even if they're small! – with your team, especially if some people are not convinced yet. We would also love to hear your stories!
 
 Some specific tips for success on the technical front:
 
-* Start with the _least_ strict "strictness" settings (which is what the default compiler options are set to) to begin, and only tighten them down iteratively.
-* Make liberal use of `any` for things you don't have types for as you go, and come back and fill them in later.
-* A good approach is to start at your "leaf" files (the ones that don't import anything else from your app, only Ember types) and then work your way back inward toward the most core types that are used everywhere.
+* Use the *strictest* strictness settings that our typings allow. While it may be tempting to start with the *loosest* strictness settings and then to tighten them down as you go, this will actually mean that "getting your app type-checking" will become a repeated process – getting it type-checking with every new strictness setting you enable! – rather than something you do just once. The only strictness setting you should turn *off* is `strictFunctionTypes`, which our current type definitions do not support. The recommended *strictness* settings in your `"compilerOptions"` hash:
+
+    ```
+    "noImplicitAny": true,
+    "noImplicitThis": true,
+    "alwaysStrict": true,
+    "strictNullChecks": true,
+    "strictPropertyInitialization": true,
+    "noFallthroughCasesInSwitch": true,
+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
+    "noImplicitReturns": true,
+    ```
+
+* Make liberal use of `any` for things you don't have types for as you go, and come back and fill them in later. This will let you do the strictest strictness settings but with an escape hatch that lets you say "We will come back to this when we have more idea how to handle it."
+
+* A good approach is to start at your "leaf" files (the ones that don't import anything else from your app, only Ember types) and then work your way back inward toward the most core types that are used everywhere. Often the highest-value modules are your Ember Data models and any core services that are used everywhere else in the app – and those are also the ones that tend to have the most cascading effects (having to update *tons* of other places in your app) when you type them later in the process.
+
+* Set `"noEmitOnError": true` in the `"compilerOptions"` hash in your `tsconfig.json` – it will help a lot if you can be sure that for the parts of your app you *have* added types to are still correct. And you'll get nice feedback *immediately* when you have type errors that way!
+
+    ![type errors in your build!](https://user-images.githubusercontent.com/108688/38774630-7d9224d4-403b-11e8-8dbc-87dad977a4c4.gif "example of a build error during live reload")
 
 You may find the blog series ["Typing Your Ember"][typing-your-ember] helpful as it walks you through not only how to install but how to most effectively use TypeScript in an Ember app today, and gives some important info on the background and roadmap for the project.
 

appveyor.yml

@@ -21,7 +21,7 @@ install:
   - yarn
 
 cache:
-#  - "%LOCALAPPDATA%\\Yarn"
+  - "%LOCALAPPDATA%\\Yarn"
 
 # Post-install test scripts.
 test_script:

lib/incremental-typescript-compiler.js

@@ -34,9 +34,11 @@ module.exports = class IncrementalTypescriptCompiler {
 
     this._buildDeferred = RSVP.defer();
     this._isSynced = false;
+    this._pendingErrors = [];
     this._triggerDir = `${this.outDir()}/.rebuild`;
     this._pendingAutoresolve = null;
     this._didAutoresolve = false;
+    this._watchProgram = null;
   }
 
   treeForHost() {
@@ -108,7 +110,7 @@ module.exports = class IncrementalTypescriptCompiler {
     let project = this.project;
     let outDir = this.outDir();
 
-    compile(project, { outDir, watch: true }, {
+    this._watchProgram = compile(project, { outDir, watch: true }, {
       reportWatchStatus: (diagnostic) => {
         let text = diagnostic.messageText;
 
@@ -135,11 +137,17 @@ module.exports = class IncrementalTypescriptCompiler {
 
       reportDiagnostic: (diagnostic) => {
         if (diagnostic.category !== 2) {
-          this.project.ui.write(ts.formatDiagnostic(diagnostic, {
+          let message = ts.formatDiagnostic(diagnostic, {
             getCanonicalFileName: path => path,
             getCurrentDirectory: ts.sys.getCurrentDirectory,
             getNewLine: () => ts.sys.newLine,
-          }));
+          });
+
+          if (this._shouldFailOnTypeError()) {
+            this.didError(message);
+          } else {
+            this.project.ui.write(message);
+          }
         }
       }
     });
@@ -163,9 +171,27 @@ module.exports = class IncrementalTypescriptCompiler {
     }
   }
 
+  didError(message) {
+    this._pendingErrors.push(message);
+  }
+
   didSync() {
     this._isSynced = true;
-    this._buildDeferred.resolve();
+    if (this._pendingErrors.length) {
+      this._buildDeferred.reject(new Error(this._pendingErrors.join('\n')));
+      this._pendingErrors = [];
+    } else {
+      this._buildDeferred.resolve();
+    }
+  }
+
+  getProgram() {
+    return this._watchProgram.getProgram();
+  }
+
+  _shouldFailOnTypeError() {
+    let options = this.getProgram().getCompilerOptions();
+    return !!options.noEmitOnError;
   }
 
   _mirageDirectory() {

lib/utilities/compile.js

@@ -23,13 +23,24 @@ module.exports = function compile(project, tsOptions, callbacks) {
     fullOptions,
     buildWatchHooks(ts.sys),
     createProgram,
-    callbacks.reportDiagnostic,
-    callbacks.reportWatchStatus
+    diagnosticCallback(callbacks.reportDiagnostic),
+    diagnosticCallback(callbacks.reportWatchStatus)
   );
 
   return ts.createWatchProgram(host);
 };
 
+function diagnosticCallback(callback) {
+  if (callback) {
+    // The initial callbacks may be synchronously invoked during instantiation of the
+    // WatchProgram, which is annoying if those callbacks want to _reference_ it, so
+    // we always force invocation to be asynchronous for consistency.
+    return (diagnostic) => {
+      process.nextTick(() => callback(diagnostic));
+    };
+  }
+}
+
 function buildWatchHooks(sys) {
   let watchedFiles = new Map();
 
@@ -48,7 +59,7 @@ function buildWatchHooks(sys) {
       if (!fs.existsSync(dir)) return;
 
       let ignored = /\/(\..*?|dist|node_modules|tmp)\//;
-      let watcher = chokidar.watch(dir, { ignored });
+      let watcher = chokidar.watch(dir, { ignored, ignoreInitial: true });
 
       watcher.on('all', (type, path) => {
         path = path.replace(/\\/g, '/'); // Normalize Windows