"tsc -b" with minimal watch capabilities #24465

RyanCavanaugh · 2018-05-29T20:48:43Z

Implements tsbuild without --watch. Opening so people have a chance to review non-watch portions sooner. Not planning to merge this unless there is schedule pressure to produce a non-watch build.

I also want to rephrase ~all of the error messages and would like suggestions on any of them

@rbuckton there's a vfs fix in here because we were re-using SourceFile instances even when the underlying file was changed

Open TODOs I'm aware of:

Nail down which parts of the tsbuilder API should be exposed (seeking feedback)
Determine exactly what --verbose output should be in all situations (seeking feedback)
Issue an error when -b appears out of order in a commandline
Issue an error when an illegal set of -b flags is provided (e.g. --clean --dry --force, which makes no sense)
Add an outFile compilation to the sample projects folder
--build --help
Support response files in --build mode (do we need this?)

rbuckton · 2018-05-29T21:05:41Z

src/harness/vfs.ts

+         *
+         * NOTE: do not rename this method as it is intended to align with the same named export of the "fs" module.
+         */
+        public utimesSync(path: string, atime: Date, mtime: Date) {


I originally had this as part of the VFS, the relevant code can be found here: https://gist.github.com/rbuckton/1d064ec108afc0479bc1457d8a46792e#file-filesystem-ts-L596

You should verify the file system isn't read-only before performing updates so that you don't inadvertently modify a copy of the FS shared between multiple tests, i.e.:

if (this.isReadOnly) throw new IOError("EROFS");

rbuckton · 2018-05-29T21:06:45Z

src/harness/vfs.ts

+                throw createIOError("ENOENT");
+            }
+            entry.node.atimeMs = +atime;
+            entry.node.mtimeMs = +mtime;


You should also set entry.node.ctimeMs = this.time() to set the status change time when you update atimeMs or mtimeMs.

…ection

…ssages

mhegazy · 2018-06-08T00:31:16Z

src/compiler/utilities.ts

                    lineStart = false;
                }
                output += s;
+                updateLineCountAndPosFor(s);


So we are walking now every string we write in the emitter to find lineStarts? why is this needed? do you know the perf impact on the emitter?

If we don't count the lines and the length of the last one, we don't set the line and column correctly for get line and column, resulting in incorrect sourcemaps.

i understand, but how often are these functions called with multi-line text? can we split these to have their own writer function to avoid doing this on every token we write?

Probably best to discuss this in #24704

mhegazy · 2018-06-08T00:45:34Z

src/compiler/tsbuild.ts

+        /**
+         * Issue a verbose diagnostic message. No-ops when options.verbose is false.
+         */
+        verbose(diag: DiagnosticMessage, ...args: any[]): void;


in other parts of the system, we have done this through host objects.

mhegazy · 2018-06-08T00:49:23Z

src/compiler/tsbuild.ts

+     */
+    function createFileMap<T>(): FileMap<T> {
+        // tslint:disable-next-line:no-null-keyword
+        const lookup: { [key: string]: T } = Object.create(/*prototype*/ null);


ES Maps has better memory characteristics than objects, since they do not hold on to the property name strings.

mhegazy · 2018-06-08T00:52:11Z

src/compiler/tsbuild.ts

+            if (arr.indexOf(element) < 0) {
+                arr.push(element);
+            }
+            if (allKeys.indexOf(key) < 0) allKeys.push(key);


consider using a map and returning keys, instead of using indexof

mhegazy · 2018-06-08T00:56:02Z

src/compiler/tsbuild.ts

+
+    function getOutputDeclarationFileName(inputFileName: string, configFile: ParsedCommandLine) {
+        const relativePath = getRelativePathFromDirectory(rootDirOfOptions(configFile.options, configFile.options.configFilePath!), inputFileName, /*ignoreCase*/ true);
+        const outputPath = resolvePath(configFile.options.declarationDir || configFile.options.outDir || getDirectoryPath(configFile.options.configFilePath!), relativePath);


looks like getDeclarationEmitOutputFilePath, can we consolidate?

mhegazy · 2018-06-08T00:58:30Z

src/compiler/tsbuild.ts

+        return changeExtension(outputPath, ".d.ts");
+    }
+
+    function getOutputJavaScriptFileName(inputFileName: string, configFile: ParsedCommandLine) {


looks like getOwnEmitOutputFilePath. can we consolidate?

Holding off on these because getOwnEmitOutputFilePath needs a SourceFile and an EmitHost and we have neither at this point.

mhegazy · 2018-06-08T00:59:06Z

src/compiler/tsbuild.ts

+    }
+
+    function getOutputFileNames(inputFileName: string, configFile: ParsedCommandLine): ReadonlyArray<string> {
+        if (configFile.options.outFile) {


there is out as well.

mhegazy · 2018-06-08T19:36:40Z

src/compiler/tsbuild.ts

+
+    function getOutFileOutputs(project: ParsedCommandLine): ReadonlyArray<string> {
+        if (!project.options.outFile) {
+            throw new Error("Assert - outFile must be set");


Debug.fail

mhegazy · 2018-06-08T19:37:49Z

src/compiler/tsbuild.ts

+            throw new Error("Assert - outFile must be set");
+        }
+        const outputs: string[] = [];
+        outputs.push(project.options.outFile);


this looks a lot like getOutputPathsFor. just use that instead.

Same problem here; those functions were written taking much wider inputs than required or present

mhegazy · 2018-06-08T19:40:06Z

src/compiler/tsbuild.ts

+    }
+
+    function isDeclarationFile(fileName: string) {
+        return fileExtensionIs(fileName, ".d.ts");


nit, use Extension.Dts instead of ".d.ts"

mhegazy · 2018-06-08T19:45:11Z

src/compiler/tsbuild.ts

+                reportDiagnostic(createCompilerDiagnostic(Diagnostics.A_non_dry_build_would_delete_the_following_files_Colon_0, filesToDelete.map(f => `\r\n * ${f}`).join("")));
+            }
+            else {
+                if (!host.deleteFile) {


seems like this should be the first check

mhegazy · 2018-06-08T19:49:28Z

src/compiler/tsbuild.ts

+            if (host.fileExists(fullPath)) {
+                return fullPath as ResolvedConfigFileName;
+            }
+            reportDiagnostic(createCompilerDiagnostic(Diagnostics.File_0_not_found, fullPath));


this will say something like c:\myconfig.json\tsconfig.json does not exist

mhegazy · 2018-06-08T19:59:21Z

src/compiler/tsbuild.ts

+            };
+            const program = createProgram(programOptions);
+
+            // Don't emit anything in the presence of syntactic errors or options diagnostics


why? we emit in the presence of errors in tsc.. and why are option errors different from semantic errors?

I see, we are not building for any errors.. nevermind then.

mhegazy · 2018-06-08T20:00:38Z

src/compiler/tsbuild.ts

+                rootNames: configFile.fileNames,
+                options: configFile.options
+            };
+            const program = createProgram(programOptions);


should not this just call inot the same routine we have in tsc.ts to perform the compilation, why does it have to be different?

mhegazy · 2018-06-08T20:02:54Z

src/compiler/tsbuild.ts

+                }
+            }
+
+            const semanticDiagnostics = [...program.getSemanticDiagnostics()];


why clone it?

mhegazy · 2018-06-08T22:34:32Z

Overall 👍 with some comments:

Emitter performance check. Please check the performance of the emitter did not regress from this change
Style comments in this PR, we can merge then handle them in a different PR.

RyanCavanaugh and others added 25 commits May 14, 2018 18:27

WIP

d98a9a0

WIP

aeb5625

WIP, need to lint

a4eb156

Lint

b2720ad

WIP

b1dedf2

Clean, etc

7e0825a

Testing WIP

6d04378

Testing WIP

76d2ba6

WIP

5a9a883

Fix errors

18f2baa

Passing tests

fa07830

Baseline accept

5a664be

WIP more tests

d4a56b9

WIP

fc22b5b

Comments

8a883ae

Add resolution branding

0781279

Fix tests run in parallel

d9bfbfe

Merge branch 'master' into tsbuild

8936b03

Fix strictNullChecks breaks

19ab8ab

Don't use invalid cached SourceFiles

cb8aa9b

Correctly skip upstream-blocked projects

8ac795b

Graph ordering test WIP

1863d3f

Use better toposorting algorithm

a7fcbcd

Add graph ordering tests

70fa29b

Merge branch 'master' into tsbuild

fe191fe

rbuckton suggested changes May 29, 2018

View reviewed changes

RyanCavanaugh added 3 commits May 29, 2018 14:28

VFS fixes

129f747

Add more errors; commandline help for --build; invalid flag combo det…

f8c4301

…ection

We definitely have a type system

cd7a844

RyanCavanaugh requested a review from DanielRosenwasser May 30, 2018 17:28

RyanCavanaugh added 7 commits June 5, 2018 16:19

Include filename when reporting not found

7ad9d57

Move proj ref logic to the right place

5111f4d

Include all lib files in tsbuild harness

11df004

Lint

856fc79

Update messages

291289f

Improve handling of container projects and issue relative filename me…

b107849

…ssages

Improved watch mode

21a65f5

mhegazy reviewed Jun 8, 2018

View reviewed changes

src/compiler/tsbuild.ts Outdated

}

}

const semanticDiagnostics = [...program.getSemanticDiagnostics()];

Copy link

Contributor

mhegazy Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why clone it?

RyanCavanaugh changed the title ~~[WIP/preview] tsbuild without -w~~ "tsc -b" with minimal watch capabilities Jun 8, 2018

RyanCavanaugh and others added 3 commits June 8, 2018 17:43

PR fixups

1de2f83

Use native map

b97bc8e

Last round PR comments

0f626fd

RyanCavanaugh merged commit f4fcb19 into microsoft:master Jun 9, 2018

RyanCavanaugh deleted the tsbuild branch June 9, 2018 17:21

mhegazy mentioned this pull request Jun 19, 2018

Support "medium-sized" projects #3469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"tsc -b" with minimal watch capabilities #24465

"tsc -b" with minimal watch capabilities #24465

RyanCavanaugh commented May 29, 2018 •

edited

Loading

rbuckton May 29, 2018

rbuckton May 29, 2018

mhegazy Jun 8, 2018

weswigham Jun 8, 2018

mhegazy Jun 8, 2018

RyanCavanaugh Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018 •

edited

Loading

mhegazy Jun 8, 2018

RyanCavanaugh Jun 9, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

RyanCavanaugh Jun 9, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy Jun 8, 2018

mhegazy commented Jun 8, 2018

"tsc -b" with minimal watch capabilities #24465

"tsc -b" with minimal watch capabilities #24465

Conversation

RyanCavanaugh commented May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Jun 8, 2018

RyanCavanaugh commented May 29, 2018 •

edited

Loading

mhegazy Jun 8, 2018 •

edited

Loading