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

Feature Request: Throw fatal error if compilation fails #10

Open
jvilk opened this issue Jan 7, 2014 · 12 comments
Open

Feature Request: Throw fatal error if compilation fails #10

jvilk opened this issue Jan 7, 2014 · 12 comments

Comments

@jvilk
Copy link

jvilk commented Jan 7, 2014

Currently, if typescript-require encounters errors during compilation but a previous compiled version of the TypeScript file is present, it will use the old one and chug on.

This is undesired behavior for me, as it's easy to miss the compilation errors and think that your changes did not fix an issue.

If this is desired default behavior, could you expose a configuration option that turns this into a fatal error?

Thanks!

@jvilk
Copy link
Author

jvilk commented Jan 7, 2014

Apparently TypeScript doesn't call process.exit like you expect, which is why your existing exception isn't triggered.

It's simple to replicate this. Just write a malformed TypeScript file, e.g. don't match a curly brace, and try to require it.

@ghost ghost assigned finalclass Jan 7, 2014
@finalclass
Copy link
Contributor

I've added exitOnError flag. Default is true.

Ohhh, we need npm publush now.

@ghost ghost assigned eknkc Jan 7, 2014
@finalclass finalclass reopened this Jan 7, 2014
@eknkc
Copy link
Contributor

eknkc commented Jan 7, 2014

Published!
Also added you in owners list, should be able to publish now.

@jvilk
Copy link
Author

jvilk commented Jan 8, 2014

This doesn't fix the issue. Here's a simple example.

example.js:

require('typescript-require');
var tsfile = require('./tsfile.ts');
console.log("I'm still running.");

tsfile.ts:

if (false) {}

Run once, and you get the following output, as expected:

$ node example.js
I'm still running.

Now, change tsfile.ts to the following malformed TypeScript:

if (false) {

Run again:

$ node example.js
/Users/jvilk/Code/doppio_lol/doppio/tsfile.ts(2,1): error TS1005: '}' expected.
I'm still running.

After inserting a debug print, TypeScript does call process.exit with code 1, but it does so after typescript-require returns from my require statement:

$ node example.js
/Users/jvilk/Code/doppio_lol/doppio/tsfile.ts(2,1): error TS1005: '}' expected.
I'm still running.
EXIT CALLED WITH CODE 1

I imagine this is the price you pay for trying to emulate a synchronous require call in an inherently asynchronous language; TypeScript's error handler probably runs in a callback somewhere.

@finalclass
Copy link
Contributor

The problem is that when that script is compiled by node synchronous method vm.runInThisContext() the execution goes further but exit callback is called later asynchonously.

Thus in this version of node it's impossible to rely on process.exit, so this code (it's taken from typescript-require index.js file) will never call exit synchronously:

 var proc = merge(merge({}, process), {
    argv: compact(argv),
    exit: function(code) {
      exitCode = code;
    }
  });

So we need a different way of knowing if there ware compilation errors.
I've got already implemented solution which rely on process.stderr. Generaly speaking when tsc calls process.stderr.write method the process exists with status code 1 and prints all the errors.
I know it's very dirty solution because it's relying on console messages but as for now I don't see any other solution on how to know if the script was compiled successfully or not.

I pushed this solution to the branch "process.stderr" please review it.

@jvilk
Copy link
Author

jvilk commented Jan 8, 2014

I can test this tomorrow... but I found an alternative solution relying on an external package, if you're interested.

execSync does exactly what you want: https://npmjs.org/package/execSync

@finalclass
Copy link
Contributor

Looks very good. I will try it later.

@ghost ghost assigned finalclass Jan 8, 2014
@svallory
Copy link
Member

execSync has a big warning right at the top of that page:

WARNING For dev machine shell scripting only. DO NOT USE for production servers

What do you guys think about using typescript-compiler to do the compilation?

@quenio
Copy link

quenio commented Dec 19, 2014

Keeping the compiler running on on its own process ensures it will not affect the execution of the program. The memory and performance requirements of the compiler could affect larger programs. That makes me think that it is better to keep the compiler isolated in its own process.

But there might be other benefits in running in in-process, such as faster execution, or better error handling, or even just-in-time compile.

@svallory
Copy link
Member

One thing doesn't affect the other. We can use typescript-compiler and still spawn a new process for the compilation. What I don't know is if spawn processes for compilation is really beneficial. Specially because on a large source base this could mean a lot of processes.

@quenio
Copy link

quenio commented Dec 19, 2014

I guess I was under the assumption that there wouldn't be so many processes since the compiler builds dependencies as well and caches them as .js files for later use.

@svallory
Copy link
Member

The require cache helps a lot. The compiler will only build dependencies if they are ///<reference>s. If you require a file inside your .ts using node's require, it will trigger typescript-require which will spawn a new process.

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

No branches or pull requests

5 participants