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

Redesign types directives #12311

Closed
nayeemrmn opened this issue Oct 3, 2021 · 4 comments
Closed

Redesign types directives #12311

nayeemrmn opened this issue Oct 3, 2021 · 4 comments
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)

Comments

@nayeemrmn
Copy link
Collaborator

Adding types to .js import-side

Current syntax

// main.ts

// @deno-types="./foo_types.d.ts"
import { foo } from "./foo.js";

// foo.js
export const foo = 1;

// foo_types.d.ts
export const foo: number;

The syntax is inconsistent with Deno's other directives and the name is vague.

Proposal

// main.ts

// deno-import-types ./foo_types.d.ts
import { foo } from "./foo.js";

// foo.js
export const foo = 1;

// foo_types.d.ts
export const foo: number;

Adding types to .js export-side

Current syntax

// main.ts
import { foo } from "./foo.js";

// foo.js
/// <reference types="./foo_types.d.ts" />
export const foo = 1;

// foo_types.d.ts
export const foo: number;

Same problems. This has no relation to the /// reference types in tsc whose syntax it borrows, bears no similarity to the import-side directive and has a vague name.

Proposal

// main.ts
import { foo } from "./foo.js";

// foo.js
// deno-export-types ./foo_types.d.ts
export const foo = 1;

// foo_types.d.ts
export const foo: number;

We should support the proposed directives. deno lint should deprecate // @deno-types and /// <reference types="<module-specifier-like>">

@kitsonk kitsonk added cli related to cli/ dir suggestion suggestions for new features (yet to be agreed) labels Oct 3, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Oct 3, 2021

Ref #11334

We wouldn't be able to eliminate old directives for a long long time, just deprecated them. Also, there are loads we can't control which we have to support, so there will always be inconsistency. So creating more inconstiancy (by depreciating one and introducing another) doesn't make sense to me personally.

Also, it isn't "export" types persay as much as just the "type" version of the runtime code. We have an issue for supporting @deno-types on re-export IIRC. So we would need to debate the names.

@nayeemrmn
Copy link
Collaborator Author

// @deno-types="..." and /// <reference types="..."> are awful and vague enough that you could swap their usages and it would make just as much or even more sense. They require different commenting styles (//, ///, @, <>, deno-) from each other and other directives because they take an arbitrarily varying amount of inspiration from tsc, even though they have confusingly no interoperability with tsc. I don't know what else would warrant deprecation.

Inconsistency across existing code from alternate legacy syntax is not the kind I'm talking about because IMO that's not much of a real problem for users, but inconsistency across idiomatic syntaxes where the two standouts are also bad for independent reasons means there exists no reasonably written way of invoking that behaviour.

Also, it isn't "export" types persay as much as just the "type" version of the runtime code.

It seems close enough. I don't think there's any other way of deferring the export types of a JS module, it's the intended purpose and the picture painted by https://deno.land/manual@v1.14.2/typescript/types#providing-types-when-hosting. From scratch I would use // deno-types for exports, but // deno-export-types is an available alternative.

@dsherret
Copy link
Member

dsherret commented Oct 4, 2021

I don't think we should make any changes to these. It's unfortunate they exist because they're not standard typescript directives. I think it would be better if we petition for something to be adopted in TypeScript itself (microsoft/TypeScript#33437).

@dsherret
Copy link
Member

Going to close this because I think we should discuss in that TypeScript issue, so let's track and discuss this there.

Note: This could be done with import attributes microsoft/TypeScript#33437 (comment)

@dsherret dsherret closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

No branches or pull requests

3 participants