handle case where '*.d.ts' files are compiled and there is no output

[ryanwe]

This fixes sporadic cases of TypeError: Cannot read property 'text' of undefined we were seeing when writing declaration files for existing CoffeeScript code. We're running TypeScript 2.0.3 with ts-node 1.3.0.

We have a fairly log repo with a combination of CoffeeScript and TypeScript. Things would work fine when compiling with tsc and using VSCode. But when running mocha tests with ts:ts-node/register as a compiler, we would occasionally see this type error.

I was able to track the problem down to getEmitOutput() returning empty output for type declaration files we added for interacting with our legacy CoffeeScript files.

This fix will allow us again to import these type declarations instead of requireing the CoffeeScript modules. I tried to repro this issue as a new test in this repository but was unable to. If you have any suggestions on how to do so or would like me to make any additional changes, let me know.

Thanks!

[coveralls]

Coverage decreased (-1.08%) to 85.953% when pulling 7c0bd62 on ryanwe:master into 991a892 on TypeStrong:master.

[coveralls]

Coverage decreased (-1.08%) to 85.953% when pulling 7c0bd62 on ryanwe:master into 991a892 on TypeStrong:master.

[coveralls]

Coverage decreased (-1.08%) to 85.953% when pulling 7c0bd62 on ryanwe:master into 991a892 on TypeStrong:master.

[blakeembrey]

Sorry, I've rejected these PRs in the past because it is wrong. ts-node should act as if you are running tsc and then node, but as a single step. This breaks that, because this code would not be able to run in node anyway if the require is failing to find a file to require. See #141 (comment).

[coveralls]

Coverage decreased (-1.08%) to 85.953% when pulling 7c0bd62 on ryanwe:master into 991a892 on TypeStrong:master.

[blakeembrey]

Does the message in https://github.com/TypeStrong/ts-node/pull/208/files make sense?

[ryanwe]

Wow! I was in the middle of responding and suggesting something similar 😄

It does not entirely make sense. For one, I'm importing a whole module, not requireing a specific type declaration file. And I do have loaders for the CoffeeScript files. This is purely (I think) about ts-node importing the type declarations so it can compile the rest of the code.

[ryanwe]

I also wanted to add that my code works with tsc followed by node as well as with VSCode and SublimeText. This problem appears to be unique to ts-node.

In my case, we have a "typings": "./path/to/file.d.ts" in our package.json in a symlinked, local module.

Thanks for the rapid response and assistance! Let me know if I can do anything to help. I do think that adding an error message is a big improvement. But it doesn't explain why we only see this problem when running our code through ts-node and mocha.

[blakeembrey]

So the important thing here is that TypeScript will load the definition anyway, that doesn't cause any errors, but this code only ever runs when the .d.ts file is being required (E.g. node has asked for it directly). So allowing this PR would mean that your code would just fail a little later instead since no runtime value has been generated (it'd end up as an empty module.exports object, unlikely to be what you want).

So how does this happen? It happens when you import any file and the .d.ts file is there instead. You could be doing require('./foo.d.ts') directly or maybe you did require('./foo.d') which resolves the .ts extension. Do you have a repro? Do you know the situation where this occurs, because I can't think of any others?

[ryanwe]

Sadly, I've been unable to repro this outside of our considerably large and complex repository. And it manifests itself at odd times. It's definitely something I'd like to understand better. And maybe we need to change how we add type declarations to our legacy CoffeeScript code.

In our case, we do have runtime values because we've previously required 'coffee-script/register' so our code runs just fine. We just want to reference the type declarations for the CoffeeScript files - kind of like a local @types or typings so we have type information and can import instead of require, since require just gives you an any.

If this isn't enough info to go on, I understand. I'll see if I can reproduce the issue in a clean repo.

[blakeembrey]

Do you know what file it's failing on also? Because there shouldn't be any case where ts-node even attempts to compile anything under node_modules/. Yes, writing the declarations makes sense but I'm 99% confident it works as I use it myself often (both by publishing TypeScript modules as JavaScript, writing definitions for existing .js projects and using ts-node) 😄 For instance, I believe the case you're running is something I do in https://github.com/pillarjs/path-to-regexp (notice the test.ts file, run by ts-node, while the actual library is written in JavaScript just with a manual .d.ts file there). The only difference is .coffee, which shouldn't change anything. If you can get the file it's failing on the import it's trying to do, that might at the very least shed some pointers.

[aseemk]

Hey @blakeembrey. I'm @ryanwe's colleague.

Great work on ts-node! And thanks for the good discussion. I agree this is pretty odd, and I see what you're saying. But I also wanted to chime in and clarify that this...

You could be doing require('./foo.d.ts') directly or maybe you did require('./foo.d') which resolves the .ts extension.

...isn't what we're doing. I'll explain our setup and hopefully that'll help.

In your https://github.com/pillarjs/path-to-regexp project, your test.ts code looks like this:

/// <reference path="typings/index.d.ts" />
...
import pathToRegexp = require('./index')

(Just FYI that the path is outdated; the project has no more typings directory.)

Our code looks instead like this:

import someModule from './someModule'

./someModule is a directory with a package.json containing "main": "./someFile.coffee" and "typings": "./someTypings.d.ts".

(I know path-to-regexp also has a package.json like this, but the test.ts file is referencing the ./index file directly, so I don't believe the package.json is looked at here.)

The key difference between our code and path-to-regexp is that we're not using /// <reference>.

/// <reference> is actually discouraged by TSLint by default these days:

http://palantir.github.io/tslint/rules/no-reference/
https://github.com/palantir/tslint/blob/3.15.1/src/configs/recommended.ts#L63

With the rationale to use ES6-style import (which we are).

And then the TypeScript manual says that for any ES6-style import, it automatically also looks for a .d.ts file based on the import's filepath — including looking at package.json "typings".

https://www.typescriptlang.org/docs/handbook/module-resolution.html#how-typescript-resolves-modules

I'm not 100% sure if this answers the open questions for why we've been encountering this, etc., but I hope this at least sheds some light.

Let us know if we can answer any more questions! And thanks again for your help.

[blakeembrey]

@aseemk The path is not outdated, I don't check in built assets - see https://github.com/pillarjs/path-to-regexp/blob/master/package.json#L16. Every project I do works like this, including ts-node 😄

The reference here is irrelevant, but that is the correct usage unless you add a tsconfig.json file (which, being a JavaScript project I retrofitted tests into, I decided to skip). Also, you might want to check out why I use import = require - that is the correct usage for importing a CommonJS module and you can't/shouldn't use a different approach of importing CommonJS code. Again, irrelevant here, but I want to fix some false assumptions.

I also understand how TypeScript resolves "typings" in package.json, I have pushed for people using that often myself. However, that should also be irrelevant because the specific code you are pointing to in this PR is only when node.js requires a .d.ts file (when TypeScript resolves it internally, that is not an error - otherwise no project with dependencies would ever work without type errors anyway).

The only thing I can do to help is hopefully get the filenames you're running into this. E.g. this is the entry, this is the require it dies on. A minimal repro would be good, because a lot of things you've pointed out aren't really relevant though the situation makes sense.

[blakeembrey]

Just to clarify further, in case I haven't emphasised this, that code never runs from TypeScript. It is only run when node.js encounters a require (which would be a transpiled import by TypeScript).

[blakeembrey]

Shoot, actually, I think I see what's going on. Give me a sec to write it out, but the PR doesn't fix it while understanding the error from https://github.com/TypeStrong/ts-node/pull/208/files, I think, would.

[blakeembrey]

Actually, ignore me, the case I was thinking of was if you had main.coffee and main.ts, depending on the order the loaders were added one would take precedence over the other. Node.js, internally, uses Object.keys orders so the last "extension" to be added to the object has the least precedent for loading - that's why .js extensions always load first. However, the .d.ts should cause this not to load here. I'll try to make a repro based on your use-case, but hopefully this gives you an idea of why I can't see how this would be happening right now 😄

Having the entry file and the require would absolutely help me. Once I release a version with that error, it might help you.

[aseemk]

Thanks for all the great explanations, @blakeembrey. They make sense.

Good news: @ryanwe may have found the cause! He's still confirming, I think, but it's likely that we are inadvertantly require'ing the .d.ts file — via a helper module I wrote a while back, requireDir.

https://github.com/aseemk/requireDir

To support e.g. CoffeeScript and other things, this module looks at require.extensions and assumes that any registered extension is fair game for require'ing.

Now that we know that's not the case for (edit: our) .d.ts, we'll either move our .d.ts file somewhere else, or similar.

I'm curious: how often do .d.ts files do more than just declare something? E.g. how often do they actually export anything?

[blakeembrey]

They can never declare anything, .d.ts files are kind of like TypeScript's versions of header files. They only give you type definitions, never any runtime values (hence requiring them doesn't make sense 😄).

[blakeembrey]

The way I always explain it is that a .ts file is basically just a .d.ts (header with definitions) combined with a .js (the runtime) file. If you wanted, you can merge them both together or split them back apart (just one is harder to maintain than the other!).

[aseemk]

Great, that's helpful, thanks. @ryanwe is patching require-dir to filter out .d.ts files, which appear to have a .ts extension to Node:

> path.extname('./foo/bar.d.ts')
'.ts'

aseemk/requireDir#42

So hopefully we're set after this. Thank you again!

[blakeembrey]

So, basically, yeah, it sucks that error existed in that case you thought should be good, but it's usually an indicator you have a bigger bug on your hands. In this case, it really wouldn't have mattered either way since you'd only be requiring for side-effects if you're loading a directory like that. I'm not sure how valuable it would be to say, yes, let's just make it pass, because if it's happening it's pretty likely you're doing something wrong (just not in this case). If anyone feels strongly about it though, definitely let me know 😄

[ryanwe]

Thanks again @blakeembrey! Glad we got this straightened up. In light of require-dir, the new error message makes sense. I agree it wouldn't necessarily have helped initially, but a good error message like this might make the next person think twice about assuming a bug in ts-node. 😄

[hatashiro]

I think we should reconsider this because now import is the recommended way to import d.ts too.

• https://palantir.github.io/tslint/rules/no-reference/

[blakeembrey]

No, it's not. Those two uses are completely different and you would never import a .d.ts file directly. That's only for the compiler to use, node would use the Javascript file. This is no different to 3 years ago either, it's not "new".

[hatashiro]

Oh, sorry that I misunderstood the rule. I now wonder what is really a right way to import a .d.ts file, e.g. global.d.ts. TSLint suggests including it in tsconfig.json but then the global.d.ts doesn't show when published into a npm package, as it's not included in the built files. Is using <reference type=...> still the right way to handle this?

[blakeembrey]

Hmm, depends. I also recommend using it with tsconfig.json. What's the use-case? Can you reasonably use declare global augmentation instead of trying to provide a global.d.ts with the library? If you can share an example with me, I can try to point out how it should be shared better 😄

[hatashiro]

Thanks. I'm writing a library to be published into npm. The directory structure is like below:

lib/
  a.ts
  b.ts
tsconfig.json
global.d.ts
package.json

Source files, a.ts and b.ts refer to types declared in global.d.ts. global.d.ts contains some types like below:

declare namespace X {
  export type A = { ... };
  export type B = { ... };
  export type C = A | B;
  ...
}

In sources, the types are used like function f(b: X.B): X.A { ... }. The sources are built into dist/ and the built index file is specified as main in package.json.

When I specify the global.d.ts in tsconfig.json, it cannot be found when installed with npm and imported with import.

What's the right way to include global.d.ts into lib/ files?

[hatashiro]

I resolved this with making index.d.ts which imports both index.d.ts and global.d.ts and then set it as types in package.json. Thanks!

[blakeembrey]

@noraesae I'd also recommend, if those types are intended for your own package, that you write it in module format instead of global. Otherwise you'd be polluting the global namespace for everyone using your project and that may create collisions. For example, I sometimes just create an interfaces.d.ts like https://github.com/blakeembrey/node-scrappy/blob/master/src/extract/interfaces.ts.