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

Reconsider the API for declaration files #2934

Closed
ry opened this issue Sep 12, 2019 · 26 comments · Fixed by #3755
Closed

Reconsider the API for declaration files #2934

ry opened this issue Sep 12, 2019 · 26 comments · Fixed by #3755

Comments

@ry
Copy link
Member

ry commented Sep 12, 2019

We currently have an invented syntax for referencing a d.ts file. Here's an example:

// @deno-types="./vendor/parser_typescript.d.ts"
import "./vendor/parser_typescript.js";

I'm not so happy with this - but not sure what to replace it with. Adding this as an API to figure out before 1.0.

@FredKSchott
Copy link

That timing is so funny, I just posted #2935. Happy to help join that with this effort, if a new API could handle both "this import has this type def" + "the current file has this type def".

@kitsonk
Copy link
Contributor

kitsonk commented Sep 12, 2019

@FredKSchott the problem with what you are suggesting in #2935 is that we need to know what to replace the JS file with in the compiler before we fetch the JS file. When retrieving a module, we could parse that module for the substitute and then go fetch the substitute before we hand it over to TypeScript, but that would be horribly inefficient.

@ry the only other option is some sort of magic guessing, which we have tried to avoid... I am all for some alternative, but we have to have an alternative that works, which basically meets this functional requirement:

  • As we are resolving the modules in the compiler, we need to definitively know what .d.ts file to feed the compiler instead of another file (usually JavaScript).

Things we probably shouldn't do:

  • Rely on automatic/magic resolution of this. One it is generally a Deno anti-pattern, two it is horribly inefficient when trying to resolve remote modules. Lots of peeking and guessing.
  • Have different rules or logic based on the module specifier. (e.g. try to load a .d.ts file when encountering a .js file)
  • Make assumptions about the relationship between .d.ts files and runtime source files. (e.g. every .js module will have a corresponding .d.ts)
  • Do something that directly conflicts with existing TypeScript behaviour (e.g. like repurpose /// <reference types="" />)
  • Keep the mapping of .d.ts in .js in some sort of external meta data (e.g. like types.json)

What we currently have implements the behaviour resolve modules, which does a couple specific Deno things (like support fully qualified module specifiers) but also "caches" the map of .d.ts to .js files so it can substitute them.

@FredKSchott
Copy link

Do something that directly conflicts with existing TypeScript behaviour (e.g. like repurpose /// )

It could be worth reaching out the TS core team, and see if there's any interest in working together on this to standardize something that suites Deno's needs and could be used by other projects as well. The only thing that I can think of is that I believe /// directives must all be at the top of the file, and couldn't be intermixed alongside imports the way // @deno-types currently is

@kitsonk
Copy link
Contributor

kitsonk commented Sep 12, 2019

Yeah, good point. I will officially open a feature request and then champion via the back channels. I can see it would be generally useful, there are cases where the implicit resolution fails and explicit resolution is needed in other situations outside of Deno.

@ry
Copy link
Member Author

ry commented Sep 14, 2019

Rely on automatic/magic resolution of this. One it is generally a Deno anti-pattern, two it is horribly inefficient when trying to resolve remote modules. Lots of peeking and guessing.
Have different rules or logic based on the module specifier. (e.g. try to load a .d.ts file when encountering a .js file)

Agree, this should be avoided.

Do something that directly conflicts with existing TypeScript behaviour (e.g. like repurpose /// )

I think this could be made to work so that the argument is a URL module specifier.

/// <reference types="./typescript.d.ts" />

I don't particularly care about being compatible with existing code... : )

Keep the mapping of .d.ts in .js in some sort of external meta data (e.g. like types.json)

maybe also a solution... maybe not.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 14, 2019

I think this could be made to work so that the argument is a URL module specifier.

/// <reference types="./typescript.d.ts" />

I don't particularly care about being compatible with existing code... : )

That already works in Deno. It just that that directive means "pull in this other type file to be evaluated as part of the context", not "instead of loading and parsing this JS file, load this .d.ts file instead". We need the latter behaviour.

The way it currently works in tsc is that it magically (Node.js resolution) probes the file system for .d.ts files before it magically probes for .ts files before it finally probes for .js files.

Also, not caring about existing code... then you end up in a situation where you can only run Deno TypeScript on Deno. We have #2644 specifically because we do care about existing code to an extend. 😁

@kitsonk
Copy link
Contributor

kitsonk commented Sep 14, 2019

@FredKSchott here is the issue I opened on microsoft/TypeScript#33437. My ramblings make sense to me, but we can see how the TypeScript team feels about it (and the wider TypeScript community).

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

@ry @kitsonk @FredKSchott
Is there any reason (other than already having d.ts files, but let's say that we are writing type definitions for an untyped JavaScript library from scratch) of using d.ts files instead of just having a TypeScript file that imports the JS symbols and re-export them as typed?

This already works in Deno. And works everywhere else.

Examples:

Untyped JS library, runtime error for bad argument:

import { caught } from 'https://raw.github.com/rsp/t.ts/master/caught.js';
caught('wrong argument');

Importing a plain TS file that reexports the library as typed, correct compile time errors:

import { caught } from 'https://raw.github.com/rsp/t.ts/master/caught.t.ts';
caught('wrong argument');

This is how caught.t.ts looks like:

import { caught as _caught } from './caught.js';

export const caught: <T>(promise: Promise<T>) => Promise<T> = _caught;

Pros:

  • the JS file stays the same
  • you import a plain TS file
  • the compiler and runtime doesn't have to support d.ts files
  • no problems with magic comments
  • the TypeScript team doesn't have to support our use case because they already do
  • no more complicated than d.ts files (I actually find it easier to understand but YMMV)
  • you can import * as x and export x as a named symbol instead of a top level object i.e. import * as _x from '...'; export const x: X = _x; and now it's imported as import { x } from '...' instead of import * as x from '...' if you like consistency (with d.ts you can't change the shape of the exported object)

Cons:

  • there is no database of t.ts types (yet)

Do you think that we could encourage using this style for writing new type definitions for Deno?

I called it t.ts (vs d.ts) files and put up a repo with info about it for future reference:

@hayd
Copy link
Contributor

hayd commented Nov 20, 2019

other than already having d.ts files

This is a big though. Could .t.ts be generated from .d.ts?

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

Could .t.ts be generated from .d.ts?

Probably not right now out of the box (like .d.ts can be generated from .t.ts with a simple tsc -d file.t.ts) but in principle should be possible to parse a .d.ts file with the typescript compiler and emit suitable code. I don't think that you can do anything in .d.ts that you couldn't do in plain TS.

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

@hayd Now I'm thinking if maybe a .d.ts file can be included in .t.ts file without much modifications.

Something like this:

import { single as _single } from './single.js';
// @ts-ignore
export const single = _single;
// .d.ts file appended here:
// @ts-ignore
declare function single(a: string): boolean;

The ts-ignore is because tsc complains about duplicate identifiers.
But it doesn't work, the type is not string=>boolean (from declare) but any=>boolean (inferred from the function body). We need a // @ts-!important pragma ;)

Maybe someone else will be able to have some idea of how to overwrite types with something that is as similar as possible to a .d.ts syntax.

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

This seems to work:

single.t.ts: (Update: fixed order in example)

// -----
// boilerplate:
import { single as _single } from './single.js';
// -----
// .d.ts file inserted here:
// @ts-ignore
declare function single(a: string): boolean;
// -----
// boilerplate:
// @ts-ignore
export const single: typeof single = _single;
// -----

and now my VS Code does what I want, i.e. shows me that the exported func is string => boolean and shows me an error, but deno runs it without error. I wonder what deno does differently that the tsc used by vs code, but (Update: this shows correct error as well) in principle it seems possible to have something like a hybrid TS import/export with appended .d.ts

For someone who'd like to experiment:

single.js:

export function single(a) {
  return a.length === 1;
}

single-test.js:

import { single } from './single.t.ts';
console.log(single(1));

(vs code underlines 1 as wrong type, but deno doesn't care - why? I don't know yet)

@kitsonk
Copy link
Contributor

kitsonk commented Nov 20, 2019

I'm not sure that is an improvement over the existing solution. This seems to require pre-processing of stuff, and not something that could be done on the fly, and would only work for Deno. The existing solution is effectively ignored by everyone else other than Deno.

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

@kitsonk the experiments above are my attempts of being able to convert existing .d.ts to .t.ts as asked by @hayd but my original idea was to write .t.ts instead of .d.ts (in case we don't have any type definitions yet, or maybe it is not hard to do because the public api of a library is small).

If there is a working .t.ts file then it should work not only for Deno (except the mandatory vs forbidden file extension in imports but that is true for all .ts files) because the result would be a plain ts file.

Edit: I am not arguing that it is the way to go to convert existing .d.ts to .t.ts files on the fly, I am just brainstorming if that is even possible at all. What I am arguing is that if we didn't have exiting .d.ts files for something then I think it would be less hassle to write .t.ts files than to write .d.ts files, because there would be no problems with special comments/pragmas and I think the idea is simpler.

@kitsonk
Copy link
Contributor

kitsonk commented Nov 20, 2019

I think we should be in a situation where someone doesn't have to produce something different. We should be able to consume .d.ts files, as currently written, or require some sort of pre-processing.

For a real world example, a solution needs to make this use-case work, using the lodash-es from Pika-cdn:

import * as _ from "https://cdn.pika.dev/lodash-es/^4.0.0";

And using the types from @types/lodash. Currently the following works in Deno today:

// @deno-types="https://unpkg.com/@types/lodash@^4.0.0/index.d.ts"
import * as _ from "https://cdn.pika.dev/lodash-es/^4.0.0";

Any replacement should be an improvement upon that, IMO.

Don't forget, some types are authored as ambient/global and some are authored as UMD/modular. Both are currently supported and should be equally supported in the future.

@rsp
Copy link
Contributor

rsp commented Nov 20, 2019

@kitsonk I agree with you. That's why my original question was "Is there any reason (other than already having d.ts files, but let's say that we are writing type definitions for an untyped JavaScript library from scratch) of using d.ts files instead of just having a TypeScript file that imports the JS symbols and re-export them as typed?"

@kitsonk
Copy link
Contributor

kitsonk commented Nov 26, 2019

I've been thinking about this and I've had a thought of what we can do.

I would still think that retaining the current functionality would be a good idea, but adding two other features to make supporting "coupling" JavaScript code with type definitions without introducing "resolution magic":

  • When fetching a remote HTTP module, Deno would support a X-TypeScript-Types header which would provide an alternative URL for the types. This would allow web servers to provide explicit resolution for Deno. For example if we were to fetch something like https://cdn.pika.dev/-/lodash-es/4.17.15/dist-es2019/lodash-es.min.js then Pika could provide a header of X-TypeScript-Types: ./lodash-es.min.d.ts.
  • When fetching a JavaScript module, it would be grepped for /// <reference types="..." /> (we would only want to do this on JavaScript files, as we need to preserve the existing TypeScript behaviour of that directive in other files. This would allow locally hosted modules to also support explicit resolution of types, without needing to have the modules remote. This of course can be applied to remote module too, if the custom header isn't easy to deliver.

In both situations, Deno would fetch and return the "alternative" module to the TypeScript compiler, with the right media types (TypeScript instead of JavaScript) as the resolved module.

@FredKSchott
Copy link

+1 for both of the above. I could implement that header on the Pika CDN if that's the direction we want to go.

@Fenzland
Copy link
Contributor

Fenzland commented Dec 4, 2019

How about add the path of .d.ts in the URL like:

import {} from 'https://localhost/foo/bar.js#d.ts=/foo/bar.d.ts';

# in URL is just a client side thing, so the server will not care about this.

@flovilmart
Copy link

@kitsonk regarding your comment was there any progress? I want to give a hand towards the 1.0 release.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 22, 2019

It is next in my backlog. I am travelling at the moment and wouldn't get a chance to 30/31st of December now. It isn't the most straight forward of change to attempt as an early issue, as it requires both changes in Rust (fairly complex ones) and changes in the TS compiler. We really need to land the public compiler API before we do any more heavy lifting on the compiler also, but that is pending review by Ry.

@flovilmart
Copy link

@kitsonk gotcha!

@kitsonk
Copy link
Contributor

kitsonk commented Jan 13, 2020

Working on this now.

@sholladay
Copy link

Am I remembering correctly that import of a .d.ts wasn't viable because of some complaining by the TypeScript compiler? Because that seems the most elegant to me...

import "./vendor/parser_typescript.d.ts"
import "./vendor/parser_typescript.js";

It's got an explicit file extension, after all. Was there any research into this? Or discussion with the TypeScript team?

@kitsonk
Copy link
Contributor

kitsonk commented Jan 19, 2020

How do you know that the first import applies to the second? Also it would be breaking for other TypeScript compilers. True we have extensions, and that isn't supported, but there are plugins that can support that. The dual import would simply not.

In the dual import, I haven't discussed with them, but it fundamentally modifies the AST, so that really isn't even viable.

@sholladay
Copy link

I would correlate them by the URL. If they have the same URL (except for extension, obviously), then they are linked, otherwise they are not. A simple rule that may not please every use case but it seems like a straightforward and reasonable rule to me. Plus it's better than custom syntax in a code comment, isn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants