Clean up some TODOs

Author: jakebailey

Diff for: Gulpfile.js

@@ -19,7 +19,8 @@ const copyright = "CopyrightNotice.txt";
 const cleanTasks = [];
 
 
-// TODO(jakebailey): This is really gross. Waiting on: https://github.com/microsoft/TypeScript/issues/25613
+// TODO(jakebailey): This is really gross. Waiting on https://github.com/microsoft/TypeScript/issues/25613,
+// or at least control over noEmit / emitDeclarationOnly in build mode.
 let currentlyBuilding = 0;
 let oldTsconfigBase;
 
@@ -123,8 +124,7 @@ const localize = async () => {
 const preSrc = parallel(generateLibs, series(buildScripts, generateDiagnostics, localize));
 const buildSrc = () => buildProject("src");
 
-// TODO(jakebailey): when should we run this? it's nice to have tests run quickly, but we also want to know if the code is broken.
-// But, if we are bundling, we are running only d.ts emit, so maybe this is fast?
+// TODO(jakebailey): add a variant of this that runs on ./built/local/tsc, with noEmit, for post-test verification.
 task("build-src", series(preSrc, buildSrc));
 
 /**
@@ -168,18 +168,16 @@ function esbuildTask(entrypoint, outfile, exportIsTsObject = false, performanceM
         bundle: true,
         outfile: performanceMatters ? preBabel : outfile,
         platform: "node",
-        // TODO: also specify minimal browser targets
-        target: "node10", // Node 10 is the oldest benchmarker.
+        target: "es2018", // This covers Node 10.
         format: "cjs",
         sourcemap: true,
-        external: ["./node_modules/*"], // TODO(jakebailey): does the test runner import relatively from scripts?
+        external: ["./node_modules/*"],
         conditions: ["require"],
         supported: {
-            // "const-and-let": false, // Unfortunately, no: https://github.com/evanw/esbuild/issues/297
-            "object-rest-spread": false, // See: https://github.com/evanw/esbuild/releases/tag/v0.14.46
+            // "const-and-let": false, // https://github.com/evanw/esbuild/issues/297
+            "object-rest-spread": false, // Performance enhancement, see: https://github.com/evanw/esbuild/releases/tag/v0.14.46
         },
-        // legalComments: "none", // TODO(jakebailey): enable once we add copyright headers to our source files.
-        // logLevel: "info",
+        // legalComments: "none", // If we add copyright headers to the source files, uncomment.
     };
 
     if (exportIsTsObject) {
@@ -239,11 +237,12 @@ const lkgPreBuild = parallel(generateLibs, series(buildScripts, generateDiagnost
 
 
 const esbuildTsc = esbuildTask("./src/tsc/tsc.ts", "./built/local/tsc.js", /* exportIsTsObject */ true, /* performanceMatters */ true);
+const writeTscCJSShim = () => writeCJSReexport("./built/local/tsc/tsc.js", "./built/local/tsc.js");
 
 
 const buildTsc = () => {
     if (cmdLineOptions.bundle) return esbuildTsc.build();
-    writeCJSReexport("./built/local/tsc/tsc.js", "./built/local/tsc.js");
+    writeTscCJSShim();
     return buildProject("src/tsc");
 };
 task("tsc", series(lkgPreBuild, buildTsc));
@@ -254,8 +253,11 @@ cleanTasks.push(cleanTsc);
 task("clean-tsc", cleanTsc);
 task("clean-tsc").description = "Cleans outputs for the command-line compiler";
 
-// TODO(jakebailey): watch mode doesn't mix with --bundle=false.
-const watchTsc = () => cmdLineOptions.bundle ? esbuildTsc.watch() : watchProject("src/tsc");
+const watchTsc = () => {
+    if (cmdLineOptions.bundle) return esbuildTsc.watch();
+    writeTscCJSShim();
+    return watchProject("src/tsc");
+};
 task("watch-tsc", series(lkgPreBuild, parallel(watchLib, watchDiagnostics, watchTsc)));
 task("watch-tsc").description = "Watch for changes and rebuild the command-line compiler only.";
 
@@ -265,15 +267,15 @@ const localPreBuild = parallel(generateLibs, series(buildScripts, generateDiagno
 // Pre-build steps to use based on supplied options.
 const preBuild = cmdLineOptions.lkg ? lkgPreBuild : localPreBuild;
 
-const esbuildServices = esbuildTask("./src/typescript/typescript.ts", "./built/local/typescript.js", /* exportIsTsObject */ true, /* performanceMatters */ true);
-
 // TODO(jakebailey): rename this; no longer "services".
 
+const esbuildServices = esbuildTask("./src/typescript/typescript.ts", "./built/local/typescript.js", /* exportIsTsObject */ true, /* performanceMatters */ true);
+const writeServicesCJSShim = () => writeCJSReexport("./built/local/typescript/typescript.js", "./built/local/typescript.js");
 const buildServicesProject = () => buildProject("src/typescript");
 
 const buildServices = () => {
     if (cmdLineOptions.bundle) return esbuildServices.build();
-    writeCJSReexport("./built/local/typescript/typescript.js", "./built/local/typescript.js");
+    writeServicesCJSShim();
     return buildServicesProject();
 };
 
@@ -289,8 +291,11 @@ cleanTasks.push(cleanServices);
 task("clean-services", cleanServices);
 task("clean-services").description = "Cleans outputs for the language service";
 
-// TODO(jakebailey): watch mode doesn't mix with --bundle=false.
-const watchServices = () => cmdLineOptions.bundle ? esbuildServices.watch() : watchProject("src/typescript");
+const watchServices = () => {
+    if (cmdLineOptions.bundle) return esbuildServices.watch();
+    writeServicesCJSShim();
+    return watchProject("src/typescript");
+};
 task("watch-services", series(preBuild, parallel(watchLib, watchDiagnostics, watchServices)));
 task("watch-services").description = "Watches for changes and rebuild language service only";
 task("watch-services").flags = {
@@ -302,10 +307,11 @@ task("dts-services", series(preBuild, buildServicesProject, dtsServices));
 task("dts-services").description = "Builds typescript.d.ts";
 
 const esbuildServer = esbuildTask("./src/tsserver/server.ts", "./built/local/tsserver.js", /* exportIsTsObject */ true, /* performanceMatters */ true);
+const writeServerCJSShim = () => writeCJSReexport("./built/local/tsserver/server.js", "./built/local/tsserver.js");
 
 const buildServer = () => {
     if (cmdLineOptions.bundle) return esbuildServer.build();
-    writeCJSReexport("./built/local/tsserver/server.js", "./built/local/tsserver.js");
+    writeServerCJSShim();
     return buildProject("src/tsserver");
 };
 buildServer.displayName = "buildServer";
@@ -321,8 +327,11 @@ cleanTasks.push(cleanServer);
 task("clean-tsserver", cleanServer);
 task("clean-tsserver").description = "Cleans outputs for the language server";
 
-// TODO(jakebailey): watch mode doesn't mix with --bundle=false.
-const watchServer = () => cmdLineOptions.bundle ? esbuildServer.watch() : watchProject("src/tsserver");
+const watchServer = () => {
+    if (cmdLineOptions.bundle) return esbuildServer.watch();
+    writeServerCJSShim();
+    return watchProject("src/tsserver");
+};
 task("watch-tsserver", series(preBuild, parallel(watchLib, watchDiagnostics, watchServer)));
 task("watch-tsserver").description = "Watch for changes and rebuild the language server only";
 task("watch-tsserver").flags = {
@@ -345,11 +354,12 @@ task("watch-min").flags = {
 };
 
 const esbuildLssl = esbuildTask("./src/tsserverlibrary/tsserverlibrary.ts", "./built/local/tsserverlibrary.js", /* exportIsTsObject */ true, /* performanceMatters */ true);
+const writeLsslCJSShim = () => writeCJSReexport("./built/local/tsserverlibrary/tsserverlibrary.js", "./built/local/tsserverlibrary.js");
 
 const buildLsslProject = () => buildProject("src/tsserverlibrary");
 const buildLssl = () => {
     if (cmdLineOptions.bundle) return esbuildLssl.build();
-    writeCJSReexport("./built/local/tsserverlibrary/tsserverlibrary.js", "./built/local/tsserverlibrary.js");
+    writeLsslCJSShim();
     return buildLsslProject();
 };
 task("lssl", series(preBuild, buildLssl));
@@ -363,9 +373,11 @@ cleanTasks.push(cleanLssl);
 task("clean-lssl", cleanLssl);
 task("clean-lssl").description = "Clean outputs for the language service server library";
 
-// TODO(jakebailey): watch mode doesn't mix with --bundle=false.
-const watchLssl = () => cmdLineOptions.bundle ? esbuildLssl.watch() : watchProject("src/tsserverlibrary");
-
+const watchLssl = () => {
+    if (cmdLineOptions.bundle) return esbuildLssl.watch();
+    writeLsslCJSShim();
+    return watchProject("src/tsserverlibrary");
+};
 task("watch-lssl", series(preBuild, parallel(watchLib, watchDiagnostics, watchLssl)));
 task("watch-lssl").description = "Watch for changes and rebuild tsserverlibrary only";
 task("watch-lssl").flags = {
@@ -382,10 +394,11 @@ task("dts", dts);
 
 const testRunner = "./built/local/run.js";
 const esbuildTests = esbuildTask("./src/testRunner/_namespaces/Harness.ts", testRunner);
+const writeTestsCJSShim = () => writeCJSReexport("./built/local/testRunner/runner.js", testRunner);
 
 const buildTests = () => {
     if (cmdLineOptions.bundle) return esbuildTests.build();
-    writeCJSReexport("./built/local/testRunner/runner.js", testRunner);
+    writeTestsCJSShim();
     return buildProject("src/testRunner");
 };
 task("tests", series(preBuild, parallel(buildLssl, buildTests)));
@@ -399,8 +412,11 @@ cleanTasks.push(cleanTests);
 task("clean-tests", cleanTests);
 task("clean-tests").description = "Cleans the outputs for the test infrastructure";
 
-// TODO(jakebailey): watch mode doesn't mix with --bundle=false.
-const watchTests = () => cmdLineOptions.bundle ? esbuildTests.watch() : watchProject("src/testRunner");
+const watchTests = () => {
+    if (cmdLineOptions.bundle) return esbuildTests.watch();
+    writeTestsCJSShim();
+    return watchProject("src/testRunner");
+};
 
 const buildEslintRules = () => buildProject("scripts/eslint");
 task("build-eslint-rules", buildEslintRules);
@@ -452,7 +468,6 @@ const esbuildTypingsInstaller = esbuildTask("./src/typingsInstaller/nodeTypingsI
 
 const buildTypingsInstaller = () => {
     if (cmdLineOptions.bundle) return esbuildTypingsInstaller.build();
-    // TODO(jakebailey): In --bundle=false, can we emit to this directly?
     writeCJSReexport("./built/typingsInstaller/nodeTypingsInstaller.js", "./built/local/typingsInstaller.js");
     return buildProject("src/typingsInstaller");
 };
@@ -586,16 +601,19 @@ task("importDefinitelyTypedTests").description = "Runs the importDefinitelyTyped
 const cleanBuilt = () => del("built");
 
 const produceLKG = async () => {
-    // TODO(jakebailey): there are probably more files here that are needed.
+    if (!cmdLineOptions.bundle) {
+        throw new Error("LKG cannot be created when --bundle=false");
+    }
+
     const expectedFiles = [
+        "built/local/cancellationToken.js",
         "built/local/tsc.js",
         "built/local/tsserver.js",
-        "built/local/typescript.js",
-        "built/local/typescript.d.ts",
         "built/local/tsserverlibrary.js",
         "built/local/tsserverlibrary.d.ts",
+        "built/local/typescript.js",
+        "built/local/typescript.d.ts",
         "built/local/typingsInstaller.js",
-        "built/local/cancellationToken.js",
         "built/local/watchGuard.js",
     ].concat(libs.map(lib => lib.target));
     const missingFiles = expectedFiles
@@ -626,8 +644,6 @@ task("generate-spec").description = "Generates a Markdown version of the Languag
 task("clean", series(parallel(cleanTasks), cleanBuilt));
 task("clean").description = "Cleans build outputs";
 
-// TODO(jakebailey): Figure out what needs to change below.
-
 const configureNightly = () => exec(process.execPath, ["scripts/configurePrerelease.js", "dev", "package.json", "src/compiler/corePublic.ts"]);
 task("configure-nightly", series(buildScripts, configureNightly));
 task("configure-nightly").description = "Runs scripts/configurePrerelease.ts to prepare a build for nightly publishing";

Diff for: scripts/dtsBundler.ts

@@ -61,8 +61,6 @@ function getDeclarationStatement(node: ts.Declaration): ts.Statement | undefined
 
 const nullTransformationContext: ts.TransformationContext = (ts as any).nullTransformationContext;
 
-// TODO(jakebailey): I can't seem to figure out how to load the real tsconfig, so, this will have to do.
-// But, why don't we get trailing commas?
 const program = ts.createProgram([entrypoint], { target: ts.ScriptTarget.ES5 });
 
 const typeChecker = program.getTypeChecker();

Diff for: scripts/produceLKG.ts

@@ -8,7 +8,6 @@ import * as glob from "glob";
 const root = path.join(__dirname, "..");
 const source = path.join(root, "built/local");
 const dest = path.join(root, "lib");
-const copyright = fs.readFileSync(path.join(__dirname, "../CopyrightNotice.txt"), "utf-8");
 
 async function produceLKG() {
     console.log(`Building LKG from ${source} to ${dest}`);
@@ -65,8 +64,6 @@ async function copyTypesMap() {
 }
 
 async function copyScriptOutputs() {
-    // TODO(jakebailey): This does not work when unbundled.
-    // TODO(jakebailey): Copyright is added by esbuild; maybe we should do it here?
     await copyFromBuiltLocal("cancellationToken.js");
     await copyFromBuiltLocal("tsc.js");
     await copyFromBuiltLocal("tsserver.js");
@@ -77,18 +74,19 @@ async function copyScriptOutputs() {
 }
 
 async function copyDeclarationOutputs() {
-    await copyWithCopyright("tsserverlibrary.d.ts");
-    await copyWithCopyright("typescript.d.ts");
+    await copyFromBuiltLocal("tsserverlibrary.d.ts");
+    await copyFromBuiltLocal("typescript.d.ts");
 }
 
 async function writeGitAttributes() {
     await fs.writeFile(path.join(dest, ".gitattributes"), `* text eol=lf`, "utf-8");
 }
 
-async function copyWithCopyright(fileName: string, destName = fileName) {
-    const content = await fs.readFile(path.join(source, fileName), "utf-8");
-    await fs.writeFile(path.join(dest, destName), copyright + "\n" + content);
-}
+// const copyright = fs.readFileSync(path.join(__dirname, "../CopyrightNotice.txt"), "utf-8");
+// async function copyWithCopyright(fileName: string, destName = fileName) {
+//     const content = await fs.readFile(path.join(source, fileName), "utf-8");
+//     await fs.writeFile(path.join(dest, destName), copyright + "\n" + content);
+// }
 
 async function copyFromBuiltLocal(fileName: string) {
     await fs.copy(path.join(source, fileName), path.join(dest, fileName));