Improve tsc watch/client signal + stderr support

[pp0rtal]

Hi!

I'm running a big app, requiring a lot of mem in watch mem (2-3GB+). I've been strugling to spot a fatal error which happens on some envs with low node compiled with a low mem profile, because this lib is hiding everything at process level.

tsc-watch and tsc-client are hiding the error for those reasons:

• tsc-watch does not print stderr of the tsc program (minor)
• tsc-watch always exit with 0
• tsc-client has by bias no way to trigger some exit event, so we can't use some exit event using Js

Current schema:

flowchart LR
A["client.js [P1]"] -->|"cp.fork()"| B("tsc-watch.js [P2]")
B --> |"cp.spawn()"| C("tsc [P3]")
B --> |"❌ exit 0"| A
C --> |"SIGABRT"| B

Loading

At ❌ we are annoyed because stderr can't be retrieved and we don't know if it was something bad.

I'm suggesting behavior to be able to pass-on signals / process code in an event.
I'm running client inside a gulp process to illustrate how we can know issues inside the software too and not only at process level.

flowchart LR
A["gulp [P0]"] -->|"client.start()"| B("tsc-watch.js [P1]")
B["client.js [P1]"] -->|"cp.fork()"| C("tsc-watch.js [P2]")
C --> |"cp.spawn()"| D("tsc [P3]")
B --> |"RFC: client.on('exit', (code, signal))"| A
C --> |"RFC: process.kill(signal)"| B
D --> |"SIGABRT"| C

Loading

RFC tsc-watch.js bahavior

Issue: behavior when tsc crashes

Here is what happen is node run out of memory:

$ node ./tsc-watch --pretty --preserveWatchOutput --onSuccess "node build"
[10:37:42 AM] Starting compilation in watch mode...

$ echo $?                                                                 
0

Expectation when tsc crashes

Here is what happen is node run out of memory:

$ node ./tsc-watch --pretty --preserveWatchOutput --onSuccess "node build"
[10:51:01 AM] Starting compilation in watch mode...


<--- Last few GCs --->

[581677:0x527e920]     2915 ms: Mark-sweep (reduce) 99.4 (100.3) -> 98.8 (101.6) MB, 4.3 / 0.2 ms  (+ 21.9 ms in 29 steps since start of marking, biggest step 3.5 ms, walltime since start of marking 42 ms) (average mu = 0.350, current mu = 0.383) allocati[581677:0x527e920]     2953 ms: Mark-sweep (reduce) 99.8 (101.1) -> 98.8 (102.1) MB, 31.0 / 0.1 ms  (+ 0.0 ms in 16 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 38 ms) (average mu = 0.279, current mu = 0.194) allocati

<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0xa389b0 node::Abort() [node]
 2: 0x96e0af node::FatalError(char const*, char const*) [node]
 3: 0xbb7a4e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xbb7dc7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xd73fd5  [node]
 6: 0xd74b5f  [node]
 7: 0xd8299b v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 8: 0xd8655c v8::internal::Heap::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node]
 9: 0xd54c3b v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node]
10: 0x109d21f v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long*, v8::internal::Isolate*) [node]
11: 0x1446379  [node]
[1]    581668 abort (core dumped)  node ./tsc-watch --pretty --preserveWatchOutput --onSuccess "node build"
$ echo $?                                                                 
134

RFC: client.js behavior

For the record, I'm using the client inside Gulp
It's Gulp duty to restart processed, but it has to be ware using a callback (with an error or no)

Issue: behavior when tsc-watch crashes

I have no way to say my worker has to be restared,
I can watch the process but it's hazardous.

function myGulpTscWatcher(cb: Undertaker.TaskCallback) {
    tsWatch.start(...);
    tsWatch.on("first_success", happyLogFunction);
    tsWatch.on("compile_errors", happyLogFunction);
    // How do I say to my `cb` something is wrong?
    // Here I'm hanging forever with a broken tsc watcher
});

Expected client event

tsc client is not supposed to close, and if it does I'd prefer be ISO with the tsc state (code, signal)
I suggest to add a .on('exit', (code: number, signal: string) => void) support which is the same prototype as a process support:

function myGulpTscWatcher(cb: Undertaker.TaskCallback) {
    tsWatch.start(...);
    tsWatch.on("first_success", happyFunction);
    tsWatch.on("compile_errors", happyFunction);
    tsWatch.on("exit", (code: any, signal: any) => {
        console.log({ code, signal })
        if (code === 0) { // Maybe in the case a process is manually killed
            cb(null); // no error
            return;
        }
        // More likely
        cb(new Error(`tsc exited with ${JSON.stringify({ code, signal })}`));
    });
});

Example if we spot a SIGABRT, we can trace

[10:57:15] 'launchTscWatch' errored after 1.21 s
[10:57:15] Error: tsc exited with {"code":null,"signal":"SIGABRT"}
    at TscWatchClient.<anonymous> (/home/portal/.../gulpfile.ts:84:12)
    at TscWatchClient.emit (events.js:400:28)
    at TscWatchClient.emit (domain.js:537:15)
    at ChildProcess.<anonymous> (/home/portal/.../tsc-watch/lib/client.js:8:48)
    at ChildProcess.emit (events.js:400:28)
    at ChildProcess.emit (domain.js:537:15)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:282:12)
    at Process.callbackTrampoline (internal/async_hooks.js:130:17)

If we manually kill tsc for instance, then code === 0 and we don't trace.

Impact

Breaking change: could break someone workflow!
Why? Maybe someone is used to not crash, or rely on the process to exit with 0 whatever the reason.

Thus, I suggest to release this in a version v5.x

ℹ️ Note I will do the PR

[pp0rtal]

Hi @gilamran
I've already fixes ready to go PR, but I created an issue for the record
If you like it, let's go ahead.

ps: thanks for your lib :)

[gilamran]

Looks very good!
lets do it in a new major version.

[gilamran]

BTW, I want to upgrade to node 12.12.x and use ESM modules (which is actually required already) in this major upgrade

[pp0rtal]

@gilamran Sure!
PR about this issue is coming soon. In any case take your time for the ESM switch

[gilamran]

@pp0rtal I've converted the project to Typescript, you can see it here: https://github.com/gilamran/tsc-watch/tree/v5
Please review if you can.

Thanks

[pp0rtal]

@gilamran Cool you initiated the project 👍

(minor note) You can put tsconfig.json at project root (alongside package.json) that usually what I see in projects and what tsc will search for by default.

More importantly, I didn't succeed to make type works :/
I'm using the following import on /client which is documented but there is an error now with types

Probably this file https://github.com/gilamran/tsc-watch/blob/v5/index.js should expose the client directly and not anything else, to match your statement in package.json "types": "types/client.d.ts",

// index.js
module.exports.TscWatchClient = require('./dist/client').TscWatchClient;

// -- other file 
// Example of working usage
import { TscWatchClient } from "tsc-watch";

But I'm afraid this won't fit a direct tsc-watch usage, I'm not sure to understand how you designed it.

What about emitting the types alongside the JS files?
You may drop the legacy index.js, and just create a lib/index.ts, and make package.json point on it.
You will be less bothered in the future

// package.json
  "exports": "./dist/tsc-watch.js",
  "main": "dist/index.js",
  "types": "dist/index.d.ts",

You lib/index.ts

#!/usr/bin/env node

export {TscWatchClient} from "./client";

// not sure tsc-watch can be imported? It's just an exposed .bin?
// import * as tscAlias from "./tsc-watch";
// export default tscAlias;

[gilamran]

Thanks, I'll check it out.

[gilamran]

I did some big changed to the structure. please review. I want to publish an alpha version so we can test it properly

[pp0rtal]

@gilamran Great! I'll do a full try by next week! 😉
I'll suggest also my changes (not a long PR) after yours relatively to this topic, I was waiting for your update.

Have a good WE

[pp0rtal]

@gilamran I gave a try to the latest v5, good job!
2 important remarks:

Client is not compiled

(dist/client/client.js is not emitted!) because:

tsc-watch/tsconfig-client.json

Line 16 in 41f58c6

"include": ["src/client/index.ts"]

should match all Ts files in the folder
src/client/**/*.ts will fix

The client can't require tsc-watch lib

Concretely it is searching inside my project, and not ths tsc-client package, see:

Error: Cannot find module '/MY_PJ_ROOT/dist/lib/tsc-watch'
Require stack:
- /MY_PJ_ROOT/node_modules/tsc-watch/dist/client/client.js
- /MY_PJ_ROOT/node_modules/tsc-watch/dist/client/index.js
- /MY_PJ_ROOT/gulpfile.ts

That's because the client can be launched with a different process folder

tsc-watch/src/client/client.ts

Line 9 in 41f58c6

const tscWatch = require.resolve(join(process.cwd(), 'dist', 'lib', 'tsc-watch'));

I advise to search tsc-watch relatively to the client:

    const tscWatch = require.resolve(join(__dirname, '..', 'lib', 'tsc-watch'));

Except this, types for the client works perfectly well 💯

[gilamran]

@pp0rtal regarding 1 I don't understand why you think that client.js is not emitted... as long as the index.ts is requiring (import) client.ts typescript will emit it. please double check it.
regarding 2, right! fixed and pushed. retry please

[pp0rtal]

@gilamran
As you are specifying to only generate the client index (src/client/index.ts) I do have only an index.d.ts,
Did you filter index.ts on purpose?

The problem is that client/index.ts requires client/client.ts file

By emitting all files type and client source are emitted

[pp0rtal]

@gilamran Hello, I don't know if my last message made sense,
but it works great if you include all client files for the tsconfig-client

As soon you merge the Ts refactor, I can open a PR about this issue.

[gilamran]

@pp0rtal That's very strange. when telling typescript to emit a file that import another file it will also emit the imported file. what are we missing here?

[pp0rtal]

@gilamran Ok I found why (I don't know the real explaination to this)
I was tryign your project inside my project/nod_modules/, when I run in it I don't have emitted files.
(Ideally I know I should have symlinked the project to link out of my node_modules)

See:

mkdir node_modules ; cd node_modules    # same bug when in any sub dir of node_modules
git clone git@github.com:gilamran/tsc-watch.git ; cd tsc-watch ; git checkout v5 ; npm install
npm run build ; ls dist/client/

index.d.ts  index.js

Why Typescript doesn't work correctly when inside a directly node_modules,
some internal security of Typescript to prevent building files not belonging to a project?

In any case you can let your config, as long you build the client.d.ts :)

[gilamran]

Yea, probably a typescript thing. treating node_modules as a special case.

Anyways, I've started converting the tests to typescript as well, so we wont have to require from ../../dist etc.
I hope that I'll have an alpha version to install via npm soon.

[gilamran]

ok, did another big change:

• Converted to jest (instead of mocha)
• Converted tests to Typescript!
  @pp0rtal Please review, I want to release an alpha

[pp0rtal]

@gilamran Just had a look to 2dac2ee and tried locally.
Test passes on my machine, and stack looks more sustainable, good job 👍

As soon you mere on master I will also open a PR to not make tsc crash silently. In any case you can release.
Thanks for all those upgrades 🚀

[gilamran]

@pp0rtal Lets move the discussion to #145