Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: --onEmit #167

Merged
merged 11 commits into from
Mar 27, 2024
Merged

feat: --onEmit #167

merged 11 commits into from
Mar 27, 2024

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Mar 26, 2024

Problem

  1. A compilation may succeed and emit nothing.
  2. A compilation may fail and still emit.

In case 1, it may be a waste to run the COMMAND, and the worst case it can cause an infinite loop. Example: app starts -> generates introspected GraphQL types -> triggers tsc -> app restarts -> ... -> triggers tsc -> no emit because unchanged -> app still restarts again due to onSuccess.

In case 2, we may still want to run the COMMAND. In fact, it is because this is such a popular behavior that noEmitOnError is defaulted to false:

This defaults to false, making it easier to work with TypeScript in a watch-like environment where you may want to see results of changes to your code in another environment before making sure all errors are resolved.

Solution

Add a --noEmit option, which may better serve use-cases such as restarting an app/server. Since TypeScript does not debounce emits, we also need to debounce ourselves with a --noEmitDebounceMs option (default 300ms).

We needed this to avoid an infinite loop, maybe it will be useful for
others.

Problem
---
If the `onSuccess` COMMAND touches any source file (e.g. generate
GraphQL types), then there will be an infinite loop.

Solution
---
TypeScript does not emit for unchanged source files, so `onEmit` will
not infinite loop. It also has added benefits:
- less noisy on success
- if `tsconfig.json` is configured to emit on error, user's app will
  still update with type errors
Copy link
Owner

@gilamran gilamran left a comment

Choose a reason for hiding this comment

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

I don't fully understand your solution, can you please provide more information?
and thanks for the PR!

Comment on lines +285 to +290
case 'run-on-emit-command':
if (emitKiller) {
emitKiller().then(runOnEmitCommand);
}
break;

Copy link
Owner

Choose a reason for hiding this comment

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

You are missing the client.ts part. and also missing tests

src/lib/args-manager.ts Outdated Show resolved Hide resolved
src/lib/args-manager.ts Outdated Show resolved Hide resolved
src/lib/tsc-watch.ts Outdated Show resolved Hide resolved
@llllvvuu llllvvuu marked this pull request as draft March 27, 2024 04:37
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Mar 27, 2024

OK, I've simplified and documented the solution, added tests, and also improved the PR description.

@llllvvuu llllvvuu marked this pull request as ready for review March 27, 2024 05:08
Copy link
Owner

@gilamran gilamran left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
I'm sorry that I'm picky, but this is complicated as it is, so I want the code to be as clear as possible.

README.md Outdated
@@ -11,6 +11,8 @@
| --------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- |
| `--onSuccess COMMAND` | Executes `COMMAND` on **every successful** compilation. |
| `--onFirstSuccess COMMAND` | Executes `COMMAND` on the **first successful** compilation. |
| `--onEmit COMMAND` | Executes debounced `COMMAND` on **every emitted file**. |
Copy link
Owner

Choose a reason for hiding this comment

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

on every emitted file? are you sure?
Does this means that if I have a complication of 100 files, this command will run 100 times?

Copy link
Owner

Choose a reason for hiding this comment

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

Also, maybe provide a use case for when someone will want to use onEmit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this means that if I have a complication of 100 files, this command will run 100 times?

No because of --onEmitDebounce. Just added a note in the README for this.

Also, maybe provide a use case for when someone will want to use onEmit?

Just added more to this line

src/lib/stdout-manipulator.ts Outdated Show resolved Hide resolved
src/lib/tsc-watch.ts Outdated Show resolved Hide resolved
@@ -71,6 +75,27 @@ function killProcesses(currentCompilationId: number, killAll: boolean): Promise<
return runningKillProcessesPromise;
}

let runningKillEmitProcessesPromise: Promise<number> | null = null;
function killEmitProcesses(currentEmitId: number): Promise<number> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure that I understand this function... can you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a comment to the function

Copy link
Owner

@gilamran gilamran left a comment

Choose a reason for hiding this comment

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

Last small changes

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lib/stdout-manipulator.ts Outdated Show resolved Hide resolved
llllvvuu and others added 5 commits March 27, 2024 03:50
Co-authored-by: Gil Amran <gilamran@gmail.com>
Co-authored-by: Gil Amran <gilamran@gmail.com>
Co-authored-by: Gil Amran <gilamran@gmail.com>
Co-authored-by: Gil Amran <gilamran@gmail.com>
@llllvvuu
Copy link
Contributor Author

thanks for the suggestions, updated

@gilamran gilamran merged commit 673fcaf into gilamran:master Mar 27, 2024
@gilamran
Copy link
Owner

Published on version 6.1.0

@llllvvuu
Copy link
Contributor Author

llllvvuu commented Mar 27, 2024

Thanks @gilamran ! It looks like the npm version doesn't have the JS changes: https://www.npmjs.com/package/tsc-watch?activeTab=code

Maybe it needs:

- "prepublish": "crlf --set=LF index.js client.js dist/**/*",
+ "prepublishOnly": "npm run build && crlf --set=LF index.js client.js dist/**/*",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants