microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.76k stars 12.46k forks source link

Some typedefs are global and some aren't in --checkJs #15901

Closed Zarel closed 7 years ago

Zarel commented 7 years ago

TypeScript Version: nightly (2.4.0-dev.20170516)

Code

These are excerpts that illustrate why it's weird:

dev-tools/globals.js

/** @typedef {{[k: string]: any}} AnyObject */

sim/dex-data.js

/** @typedef {{[k: string]: any}} AnyEffect */

sim/dex.js

/** @type {AnyObject} */
let a; // this isn't an error
/** @type {AnyEffect} */
let b; // this IS an error

tsconfig.json

{
    "include": [
        "./dev-tools/globals.ts",
        "./sim/dex-data.js",
        "./sim/dex.js"
    ]
}

These are excerpts that explain why this behavior is so weird, but the real reason why it works this way is probably in the rest of the code, which you can see for yourself in: https://github.com/Zarel/Pokemon-Showdown/tree/149ca3759c141085d2f473fc9fdc5da5371ef32c

Expected behavior: No error, or perhaps two errors.

Actual behavior:

sim/dex.js(3,12): error TS2304: Cannot find name 'AnyEffect'.

It's actually massively confusing to me how to access types defined in other files (I've googled and can't find any documentation on it). The general impression I get is that in modern TypeScript versions, in .ts files, you just export them and import them and they'll work normally, but still nothing on how to do it in .js files.

But either way, this seems like a bug.

(I tried moving the AnyEffect typedef from sim/dex-data.js to dev-tools/globals.js and I still get an error for it but not AnyObject, so there is almost certainly something weird going on here.)

mhegazy commented 7 years ago

There are a few issues here. first dex-data.js is a node module, and thus has its own scope. so a declaration within dex-data.js would not be visible in dex.js unless it is exported.

second, a @typedef is not parsed unless it is "associated" with some node, be it a variable declaration or a statement. this is a bug that we need to fix.

So a work around would be to move /** @typedef {{[k: string]: any}} AnyEffect */ to the top of dex.js should do the trick.

Zarel commented 7 years ago

@mhegazy this does not seem to explain why adding

/** @typedef {{[k: string]: any}} AnyEffect */
/** @type {AnyEffect} */
var a = {};

to dev-tools/globals.js does not suppress this error.

Also, does TypeScript support exporting declarations from .js files? I'm using AnyEffect as a test, the real interfaces I'm trying to export are rather complicated and used in multiple files.

sandersn commented 7 years ago

A much better workaround is to put your interfaces in a .d.ts file. There's no point in having a js file with nothing but comments, and if you use a .d.ts, then your package is ready for use from typescript if needed.

Zarel commented 7 years ago

That's not an ideal workaround because it seems the best pattern is to define interfaces in the file with the function that creates them.

For instance, say I have a file:

/** @typedef {isOn: boolean, timeLeft?: number} TimerState */
/** @return TimerState */
export function getTimerState() {...}

Other files that use getTimerState's return value would need to access TimerState, but it seems like the interface and function should be defined in the same file.

sandersn commented 7 years ago

It's actually a better design to have TimerState in a separate file and then have both the implementer (getTimerState) and usage references the separate file. For a small project, a file called types.d.ts usually works for me. The type definitions can serve as the API for your code as well.

Disclaimer: I have been working on compilers for a long time, and maybe the best design for a compiler is different from other categories of software.

Zarel commented 7 years ago

I mean, I can understand the need for header files, which .d.ts files seem analogous to. It's just... we haven't had to manually write header files since C/C++. This is what #7546 is tracking.

Honestly, though, not having header files at all is the current standard, at least my impression is that most modern code is written by just importing modules directly.

sandersn commented 7 years ago

I agree, this should work because it does work using typescript syntax in a typescript file. It's definitely a bug. I was just proposing a workaround to use until the bug is fixed.

Not using header files is facilitated by classes, which declare a type and a value together. Maybe a better workaround for you would be to define a class TimerState instead of using a @typedef?

Zarel commented 7 years ago

That doesn't work; classes can't be exported either.

See #14056 and #15720

sandersn commented 7 years ago

Fix is up at #16488