Implement streamed input with `tsc -w` capacity #15

nwetzel22 · 2023-03-12T13:13:35Z

These changes introduce input streaming from tsc output with the ability to handle tsc -w. Description of major file changes/additions:

index.ts

Instantiates a readline interface and delegates to program.ts for event handling

program.ts

changes to a class to hold state
exposes public methods for index.ts to delegate to when encountering line and close events

error-store.ts

a repository for errors parsed by program.ts
indexes errors via flags that indicate match/classification results (e.g. error code, file match)

watch-mode.spy.ts

"Spies" on tsc output looking for specific lines to determine if we're in watch mode
It would be nice to have some integration tests for this since it's vulnerable to TS changing watch mode outputs.

classify-tsc-error.ts

Reads a parsed tsc error and classifies it with match code flags

update-config-files.ts

Just a wrapper of config generating methods to reduce bloat in program.ts

process-compilation-results.ts

Another wrapper of code that handles results of a completed round of tsc compiling

Other touches of files are mainly adding periods to the end of lines. Sorry for the PR pollution!

Gelio · 2023-03-12T17:26:25Z

Thanks for the PR! I appreciate you also adding tests 🎉 I will take a closer look at the code tomorrow.

Just eyeballing the diff from that single commit, I see it includes some stylistic changes (adding dots at the end of logs/error messages) with functional changes (adding stdin streaming). Could you split it into 2 commits, so the commits are more atomic?

Moreover, I would appreciate if you followed the conventional commits convention I have been using in the project. That would make those commit messages fit better

nwetzel22 · 2023-03-12T19:05:59Z

Happy to make those changes :)

Gelio

I am almost done with the review. I have just a handful of files to review. I will finish it tomorrow or on Wednesday. Overall, great work! I particularly like the idea to use flags to compress the 4 possible variants of an error 🚀

I added some mostly-minor comments. The comments I left earlier still apply (but don't worry, there is no pressure to address them in a time-sensitive way)

Gelio · 2023-03-13T19:50:17Z

src/cli/config/update-config-files.ts

+  );
+};
+
+export default updateConfigFiles;


This project uses named exports. Could you use named exports in the code that you add as well? I want to maintain consistency in the project.

Gelio · 2023-03-13T19:55:09Z

src/cli/config/update-config-files.ts

+) => {
+  updateLooselyTypeCheckedFilePaths(
+    cliDependencies,
+    new Set(looselyTypeCheckedFilePaths),


Minor:

Looks like we should be able to avoid the unnecessary set copying

Suggested change

new Set(looselyTypeCheckedFilePaths),

looselyTypeCheckedFilePaths,

Gelio · 2023-03-13T20:33:22Z

src/cli/program.ts

+
+    if (tscError !== this.lastTscError) {
+      const errorClassification = classifyTscError(
+        tscError as TscError,


We could avoid a potentially-unsafe type assertion by adding a && tscError !== undefined in the condition of the if above. I don't think it would hurt the if in any way, and it would make it possible to remove the 2 as type assertions here and 5 lines lower, which I'm always happy to do

Gelio · 2023-03-13T20:38:45Z

src/tsc-errors/classify-tsc-error.ts

+  const errorCodeMatch = ignoredErrorCodesSet.has(tscError.tscErrorCode);
+  const filePathMatch = filePathMatcher.matches(tscError.filePath);
+
+  let matchFlags = TscErrorMatchFlags.filePath & TscErrorMatchFlags.errorCode;


Very clever way of using bit flags instead of having 4 separate buckets like I had before 🎉

Minor:

Could you set this to 0 explicitly? I had to think a bit why this is initialized the way it is, and then we use bitwise or. Only after a moment it occurred to me that this is simply a verbose way of initializing this to 0

Suggested change

let matchFlags = TscErrorMatchFlags.filePath & TscErrorMatchFlags.errorCode;

let matchFlags = 0;

I agree that it's a bit cryptic. I considered a none = 0 flag but ultimately leaned away from it because it was more of a pseudo-flag than an actual flag. I can resurrect that or find a better way to indicate the absence of any flags. Setting it to 0 explicitly works for me as well.

IMO a none = 0 case works well. Alternatively, add a const noMatchFlags = 0; next to the enum definition and use that as the initial value here. I reckon that would be less surprising and would also link the 2 parts of code together

Gelio · 2023-03-13T20:40:31Z

src/tsc-errors/classify-tsc-error.test.ts

+    );
+
+    expect(errorClassification).toBe(
+      TscErrorMatchFlags.errorCode & TscErrorMatchFlags.filePath,


Can you just say 0 instead of a bitwise and of 2 values that compute to 0? IMO just 0 is less confusing and less typo prone

Suggested change

TscErrorMatchFlags.errorCode & TscErrorMatchFlags.filePath,

0,

Gelio · 2023-03-13T20:48:34Z

src/watch-mode-spy.test.ts

@@ -0,0 +1,65 @@
+import WatchModeSpy from './watch-mode-spy';
+
+describe('watchModeSpy', () => {


Regarding writing integration tests, you could add such integration tests in jest, if you feel like it. It would involve:

Creating a new test temporary directory

Starting a new tsc process in watch mode

and then, depending on the level of integration you want the tests to be at, you could either:

Read the stdout of that tsc process and invoke corresponding WatchModeSpy methods, making assertions such as:

Initially, it should detectWatchModeCompilationStartOutput and detectWatchModeCompilationFinishedOutput

After the code modifies a file in the filesystem, it should detectWatchModeFileChangeDetectedOutput and then detectWatchModeCompilationFinishedOutput

This sounds testable. It tests WatchModeSpy kind of in isolation from the rest of the program, but that is ok. It gives us confidence that this part works, and the rest of the program is tested with existing integration tests.

Pass the output directly to program and make assertions about the program's output

This is more involved, gives us more confidence that the entire tool works end-to-end. If there are problems, it will be very coarse and not give us precise signs where the root of the problem is, just that something is broken.

I believe testing WatchModeSpy in isolation against the output of tsc -w is good enough.

It is also fine if you want to skip adding these tests in this PR. They can be added later, either by me or by you, or by some other contributor

nwetzel22 · 2023-03-14T02:14:51Z

Appreciate the review comments. I'll implement the changes sometime this week and also adjust the commits to comply with repo standards.

Gelio

I've reviewed the entire PR. I will say that again, great job! I have yet to run it myself to assess how it works for me with watch mode. It looks very promising, though!

I want to emphasize that the review would be much easier if the changes that extract unmodified existing parts of the code were separated from the commits that add new features or modify the existing logic. Having these in individual commits would be easier to see how the changes are related to one another. Right now when I was reviewing the code I had to pay close attention to the methods like generateReport or parseTscLine to see if this is new code or existing code in a different packaging.

I'm saying that to make you aware how it looks from the reviewer's perspective

I'm looking forward to seeing more changes in this PR

Gelio · 2023-03-14T07:03:12Z

src/tsc-errors/error-store.ts

+class ErrorStore {
+  private tscErrors: Map<TscErrorMatchFlags, TscError[]> = new Map();
+
+  public getAllErrors(): TscError[] {


This method seems to only be used in the tests. If we don't need it in production code, let's remove it

Gelio · 2023-03-14T07:06:33Z

src/tsc-errors/error-store.ts

+    return errors;
+  }
+
+  public getValidErrors(): TscError[] {


Minor:

All errors are valid, but some are ignored, and some are not ignored. Can we use the name

Suggested change

public getValidErrors(): TscError[] {

public getUnignoredErrors(): TscError[] {

or use some other word to avoid the confusion around calling them valid errors 😅

I know this probably was the name used in my initial implementation, but this occurred to me as I was reading this code. Feel free to skip this suggestion

Gelio · 2023-03-14T07:07:57Z

src/tsc-errors/error-store.ts

+    this.tscErrors.get(errorMatchFlags)?.push(error) ||
+      this.tscErrors.set(errorMatchFlags, [error]);


Quite clever syntax for pushing to an array with a fallback when the key does not exist. I like it 👍

Gelio · 2023-03-14T07:10:16Z

src/tsc-errors/error-store.test.ts

+    expect(errorStore.getCouldBeIgnoredErrors()).toHaveLength(1);
+  });
+
+  it('should reset buckets.', () => {


Minor:

You may as well combine these 2 tests by adding the reset operation at the end of the previous test and asserting buckets are empty. I'm suggesting this because these tests share the same setup code, so they share quite a bit of assumtpions

Gelio · 2023-03-14T07:11:29Z

src/tsc-errors/error-store.ts

+import { TscError, TscErrorMatchFlags } from './types';
+
+class ErrorStore {
+  private tscErrors: Map<TscErrorMatchFlags, TscError[]> = new Map();


Minor:

May as well throw a readonly here to prevent accidental reassigning of this property

Implement streamed input with tsc -w capacity

2b0638d

nwetzel22 mentioned this pull request Mar 12, 2023

Add stream parsing and processing support #4

Open

Gelio reviewed Mar 13, 2023

View reviewed changes

Gelio reviewed Mar 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement streamed input with `tsc -w` capacity #15

Implement streamed input with `tsc -w` capacity #15

nwetzel22 commented Mar 12, 2023 •

edited

Loading

Gelio commented Mar 12, 2023

nwetzel22 commented Mar 12, 2023

Gelio left a comment

Gelio Mar 13, 2023

Gelio Mar 13, 2023

Gelio Mar 13, 2023

Gelio Mar 13, 2023

nwetzel22 Mar 14, 2023 •

edited

Loading

Gelio Mar 14, 2023

Gelio Mar 13, 2023

Gelio Mar 13, 2023

nwetzel22 commented Mar 14, 2023

Gelio left a comment

Gelio Mar 14, 2023

Gelio Mar 14, 2023

Gelio Mar 14, 2023

Gelio Mar 14, 2023

Gelio Mar 14, 2023

	new Set(looselyTypeCheckedFilePaths),
	looselyTypeCheckedFilePaths,

	let matchFlags = TscErrorMatchFlags.filePath & TscErrorMatchFlags.errorCode;
	let matchFlags = 0;

	TscErrorMatchFlags.errorCode & TscErrorMatchFlags.filePath,
	0,

		@@ -0,0 +1,65 @@
		import WatchModeSpy from './watch-mode-spy';

		describe('watchModeSpy', () => {

	public getValidErrors(): TscError[] {
	public getUnignoredErrors(): TscError[] {

		this.tscErrors.get(errorMatchFlags)?.push(error) \|\|
		this.tscErrors.set(errorMatchFlags, [error]);

Implement streamed input with tsc -w capacity #15

Are you sure you want to change the base?

Implement streamed input with tsc -w capacity #15

Conversation

nwetzel22 commented Mar 12, 2023 • edited Loading

Gelio commented Mar 12, 2023

nwetzel22 commented Mar 12, 2023

Gelio left a 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

nwetzel22 Mar 14, 2023 • 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

nwetzel22 commented Mar 14, 2023

Gelio left a 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

Implement streamed input with `tsc -w` capacity #15

Implement streamed input with `tsc -w` capacity #15

nwetzel22 commented Mar 12, 2023 •

edited

Loading

nwetzel22 Mar 14, 2023 •

edited

Loading