Skip to content

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

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

Closed
Zarel opened this issue May 17, 2017 · 9 comments
Closed

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

Zarel opened this issue May 17, 2017 · 9 comments
Assignees
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation Fixed A PR has been merged for this issue

Comments

@Zarel
Copy link

Zarel commented May 17, 2017

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
Copy link
Contributor

mhegazy commented May 17, 2017

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.

@mhegazy mhegazy added the Bug A bug in TypeScript label May 17, 2017
@mhegazy mhegazy added this to the TypeScript 2.4 milestone May 17, 2017
@mhegazy mhegazy added Domain: JSDoc Relates to JSDoc parsing and type generation Salsa labels May 17, 2017
@Zarel
Copy link
Author

Zarel commented May 18, 2017

@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
Copy link
Member

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
Copy link
Author

Zarel commented May 20, 2017

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
Copy link
Member

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
Copy link
Author

Zarel commented May 24, 2017

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
Copy link
Member

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
Copy link
Author

Zarel commented May 24, 2017

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

See #14056 and #15720

@sandersn
Copy link
Member

Fix is up at #16488

@sandersn sandersn added the Fixed A PR has been merged for this issue label Jun 13, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: JSDoc Relates to JSDoc parsing and type generation Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

3 participants