Warn on `noEmitHelpers: true` because is likely to cause problems; causes unhelpful error

lobsterkatie commented 2 years ago

Search Terms

ReferenceError: __values is not defined tslib noEmitHelpers importHelpers

Expected Behavior

An error message which helps me solve the problem. Could be something along the lines of what's described in https://github.com/TypeStrong/ts-node/issues/1504 (When transpiled code attempts to require tslib, regenerator-runtime, or @swc/helpers, log a helpful warning about adding those dependencies; link to relevant docs page), making sure to include this case in the docs, or could be logic to actually detect this situation and suggest a solution.

Specifically, the problem here is that noEmitHelpers means tslib helpers aren't included during compilation. Normally that's fine, as long as you have importHelpers turned on, but if you have no other imports or exports (require calls don't count), importHelpers does you no good, even with tslib added as an explicit dependency. The solution (in my case) was to convert my requires to imports, which I hadn't yet gotten around to in converting my script from JS to TS. (Presumably turning off noEmitHelpers would also work.) This is the comment which finally saved me.

Actual Behavior

The raw error, with no explanation, not even that this is a tslib problem:

/Users/Katie/Documents/Sentry/forks/ts-node-repros/example.ts:1
for (const num of [1, 2, 3]) {
                  ^
ReferenceError: __values is not defined
    at Object.<anonymous> (/Users/Katie/Documents/Sentry/forks/ts-node-repros/example.ts:1:19)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Module.m._compile (/Users/Katie/Documents/Sentry/forks/ts-node-repros/node_modules/ts-node/src/index.ts:1455:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Object.require.extensions.<computed> [as .ts] (/Users/Katie/Documents/Sentry/forks/ts-node-repros/node_modules/ts-node/src/index.ts:1458:12)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at phase4 (/Users/Katie/Documents/Sentry/forks/ts-node-repros/node_modules/ts-node/src/bin.ts:567:12)
    at bootstrap (/Users/Katie/Documents/Sentry/forks/ts-node-repros/node_modules/ts-node/src/bin.ts:85:10)
error Command failed with exit code 1.

Steps to reproduce the problem

Create a script with no imports or exports, which needs a tslib helper when down-compiled
Set noEmitHelpers set to true in your tsconfig
Try to run the script with ts-node
Get the error above

Minimal reproduction

example.ts:

// possibly `require` calls, but no imports

for (const num of [1, 2, 3]) {
  console.log(num);
}

tsconfig.json:

{
  "compilerOptions": {
    "downlevelIteration": true,

    // this isn't necessary to cause the error but does make the problem harder to track down
    // "importHelpers": true,

    "noEmitHelpers": true
  }
}

Then run:

yarn add -D ts-node typescript
yarn add -D tslib # not necessary to cause the error but also makes it harder to figure out
ts-node example.ts

Specifications

Versions:

> yarn ts-node -vv             
ts-node v10.7.0
node v16.14.0
compiler v4.6.2

tsconfig.json:

{
  "compilerOptions": {
    "downlevelIteration": true,

    // this isn't necessary to cause the error but does make the problem harder to track down
    // "importHelpers": true,

    "noEmitHelpers": true
  }
}

Operating system and version: OS X 12.2.1

cspotcode commented 2 years ago

What's the use-case for setting noEmitHelpers? Is this a project that's largely frontend, but with a few ts-node scripts?

I've been hesitant to add error-detection logic because I'm not sure of the performance impact nor of our ability to reliably intercept errors in nodejs.

If we can perform some sort of validation check on your configuration at bootup, and warn when your tsconfig has this flag set, I think that might be easier to implement, have better performance, and be just as useful to you.

cspotcode commented 2 years ago

There are a bunch of issues that can be detected by setting isolatedModules. I find myself recommending it a lot. So that's one option: on startup, we detect known problematic configurations and emit a warning that suggests enabling isolatedModules. Then the TS typechecker will warn you about global scripts, suggesting that you add export {} to make them a module.

If you're using swc or transpileOnly, then the typechecker doesn't run. So an alternative is that we detect any time you've set noEmitHelpers and warn against using it. We can suggest a ts-node-specific override: https://typestrong.org/ts-node/docs/configuration#via-tsconfigjson-recommended

We might also be able to force noEmitHelpers: false but I'm not sure if there are any legitimate use-cases for setting noEmitHelpers: true in ts-node.

lobsterkatie commented 2 years ago

What's the use-case for setting noEmitHelpers? Is this a project that's largely frontend, but with a few ts-node scripts?

Yes, exactly. https://github.com/getsentry/sentry-javascript

(We're backend, too, of course, but you're correct that noEmitHelpers is about our browser bundle.)

I've been hesitant to add error-detection logic because I'm not sure of the performance impact nor of our ability to reliably intercept errors in nodejs.

I can't speak to the performance aspect, but I do know a thing or two about intercepting errors! Happy to consult if you decide to go that route. I think it could be as simple as wrapping the global error handler (and/or try-catching whatever code throws here), checking the message for the names of any of the tslib functions (there aren't that many), and pointing people to a docs page with tslib troublshooting.

There are a bunch of issues that can be detected by setting isolatedModules.

Yup. We're actually in process of transitioning to that right now! https://github.com/getsentry/sentry-javascript/pull/4497 That's on a branch, but will get pulled in soon. Interesting, though. So I wouldn't have run into this at all had I happened to be working on my current PR three weeks from now? Good to know, LOL.

If we can perform some sort of validation check on your configuration at bootup, and warn when your tsconfig has this flag set, I think that might be easier to implement, have better performance, and be just as useful to you.

Sure, that could work, too!

Thanks for taking a look at this!

cspotcode commented 2 years ago

I can't speak to the performance aspect, but I do know a thing or two about intercepting errors! Happy to consult if you decide to go that route.

Yeah, any wisdom you can share would be great. I did a bit more thinking about this. If we went the error-catching route, we'd need to catch stuff that happens async as well.

setTimeout(function() {
  for (const num of [1, 2, 3]) {}
}, 1000)

By "global error handler", are you referring to process.on('uncaughtExceptionMonitor')? https://nodejs.org/dist/latest-v17.x/docs/api/process.html#event-uncaughtexceptionmonitor

It appears to emit before the error? So I guess from the user's perspective, they'd see our warning appear just above the error.

I think, in the interest of reducing implementation effort and complexity, I'll go for validating the tsconfig, detecting noEmitHelpers: true, and logging a warning for that.

But there are a bunch of other warning ideas in #1504, and some of those might be better candidates for intercepting and matching well-known error messages.

lobsterkatie commented 2 years ago

By "global error handler", are you referring to process.on('uncaughtExceptionMonitor')?

I was thinking about uncaughtException. (The process is ending, so it's a valid use by the docs' standards.) If you're curious what we do with it, you can check it out here. (For that matter, I suppose you could even just (ab)use our SDK, let it catch and process the errors, and then just choose never to send them to us.)

I think, in the interest of reducing implementation effort and complexity, I'll go for validating the tsconfig, detecting noEmitHelpers: true, and logging a warning for that.

Sounds good!

TypeStrong / ts-node