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

Support d.ts files #1432

Closed
reggi opened this issue Dec 31, 2018 · 26 comments · Fixed by #2746
Closed

Support d.ts files #1432

reggi opened this issue Dec 31, 2018 · 26 comments · Fixed by #2746

Comments

@reggi
Copy link

reggi commented Dec 31, 2018

I have a simple example here it's a .js file paired with a d.ts file and deno doesn't throw any type issues where typescript does.

import lodash from './deno_modules/lodash.js'
console.log(lodash.add('2', '2'))

screen shot 2018-12-30 at 11 57 37 pm

screen shot 2018-12-30 at 11 57 51 pm

Typescript does throw an issue here when run within the node runtime.

example.ts(2,24): error TS2345: Argument of type '"2"' is not assignable to parameter of type 'number'.

@reggi reggi changed the title No typing for js files No typing check for js files Dec 31, 2018
@kitsonk
Copy link
Contributor

kitsonk commented Jan 1, 2019

Deno does not resolve .d.ts files currently when trying to load a .js file. It simple type checks the .js file with the "CheckJS" feature of TypeScript which limits the type checking to just what is in JSDoc.

There is a whole ugly logic on .d.ts resolution we wouldn't want to tackle. Even if we simplified to it to a 1/1 .js to .d.ts (which usually doesn't represent real world stuff anyways) there is a huge overhead then when loading a .d.ts. While it isn't perfect, I think the focus should be on authoring type safe code in TypeScript for use in Deno then trying to import other libraries which have type definitions.

@reggi
Copy link
Author

reggi commented Jan 5, 2019

@kitsonk I've been working on a reasonable and sane way to port node modules to deno, and it's unfortunate that this .js / .d.ts relationship works with node and typescript and not deno because deno needs it more. My understanding of how @type/ modules and namespaces work are all centered around the npm package names.

The issue I keep facing is that there's no way I know of to pair, merge, calculate the resolve of a module for instance lodash and it's type module @type/lodash, except for the one way this issue brings up.

Unless...

  • Is there a way for deno to ignore type checking a .ts file?
  • Does .ts / .d.ts relationship work in deno?
  • Does // @ts-ignore work? I don't believe it does after some testing.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 5, 2019

  • Is there a way for deno to ignore type checking a .ts file?

Why? I don't see why this would make any sense. It is a file is attesting it is compliant with the superset of syntax that TypeScript supports. Error in this file are errors. It would be dangerous and unsafe to run them.

  • Does .ts / .d.ts relationship work in deno?

There is no such thing. .d.ts is a TypeScript without any functional code. A .ts file includes functional code. There is no logical relationship.

  • Does // @ts-ignore work? I don't believe it does after some testing.

It should. Could you provide an example where it doesn't?

@luvies
Copy link
Contributor

luvies commented Jan 27, 2019

I've been thinking about writing stuff that would interop with standard JS, and I feel like it would be very annoying (long term especially) to be forced to not use .d.ts files. My main concern would be with how some JS is written, in that older styles tend to use more implicit mixins (this is a pattern I personally don't like, but it exists). A primary example would be the AWS SDK, which is build in such a way that it would be very difficult to get reasonable type definitions implicitly, which would (if the SDK worked at all with deno) make the implicit typings nearly impossible to use.

I do think that it would be worth writing deno libraries from scratch in TS to properly support the platform, but you can't expect everyone to have the time or patience to rewrite entire existing libraries just to allow proper typing. Being able to port from existing Node packages to a deno module would be invaluable, and supporting .d.ts would definitely help with that.

The only solutions I can think of is to provide a similar system to how the // @ts-ignore and other compiler directives work. To use the example given above:

// @ts-defs ./deno_modules/lodash.d.ts
import lodash from './deno_modules/lodash.js'
console.log(lodash.add('2', '2'))

It keeps with the explicit style that deno is after, but would require a little bit of middleware in the compiler to support, but could be possible by clipping into the same logic that compilerOptions.paths uses I would have thought. It wouldn't be supported by editors, but, from what I can gather, not a lot of how deno works is, so it's a non-issue at this point. A bonus of something like this is that it would support

// @ts-defs https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/lodash/index.d.ts
import lodash from './deno_modules/lodash.js'
console.log(lodash.add('2', '2'))

Allowing (some) compatibility with the DefinitelyTyped project.

@ry
Copy link
Member

ry commented Jan 27, 2019

We should definitely support .d.ts files... maybe initially by just looking for sibling files in the same directory ?

@luvies
Copy link
Contributor

luvies commented Jan 28, 2019

For initial support, that would work, but it shouldn't be a permanent thing, since it goes against the general design, I feel. We would also need to consider remote .d.ts files, do we try and resolve those? If so, that would require implementing a request check that borders on what we didn't want to do for source files. Plus, if it's going to be dropped for a better system in future, it feels unnecessary to do (so local only for now seems to be better).

@kitsonk
Copy link
Contributor

kitsonk commented Jan 28, 2019

We should definitely support .d.ts files... maybe initially by just looking for sibling files in the same directory ?

I don't think you will every be happy with the performance implications. We killed extensionless partly because of this. Just consider for a moment, you load a plain .js file deno https://example.com/foo.js, we would have to have it so that Deno would also do a fetch on deno https://example.com/foo.d.ts.

Some sort of comment pragma like @luvies suggest is about the only thing I can think of that would work cleanly, and follows the more explicit nature of doing this. 1:1 mapping of .js to .d.ts siblings is also not a very common way of generating .d.ts files. Likely the least common way except when you are transpiling from TypeScript, which hopefully you are pointing at the .ts sources anyways.

I would look to try to stay away from comment pragma like // @ts- as that that is the domain of TypeScript proper, I would say we would want to do something like // @deno-defs but I think I generally agree with everything else @luvies suggests.

@luvies
Copy link
Contributor

luvies commented Jan 28, 2019

I see that, // @deno-defs I think is good, since for now it would only be a deno thing. I think my angle was that, we may need to fork the TS compiler at some point to support everything we want properly, so it might be good to try and contribute to the repo directly. Something like adding a "module": "deno" option to the tsconfig seems like a reasonable thing to do to aid editor support (we could also add "lib": ["deno"] to help with intellisense), and a // @ts-defs option might help outside of a deno environment.

@ry
Copy link
Member

ry commented Jan 28, 2019

I don't think you will every be happy with the performance implications.

Good point - I hadn't considered that. I guess if a program is completely JS it should be able to avoid the check for d.ts and only in the situation where TS imports JS would it would look for the sibling d.ts file - that doesn't seem too bad?

(I'm not against special comments - I actually think it's a very nice way to augment script behavior... but for minimalism's sake I will play the antagonist role and argue against it.)

@reggi
Copy link
Author

reggi commented Jan 28, 2019

Just want to mention that typescript does implement something like this, special comments + imports as Triple-Slash Directives.

/// <reference path="..." />

Deno could implement using the XML spec as they've outlined, perhaps even a new attribute or tag, or reusing <reference>?

Deno would need to support this anyway because .d.ts files support this.

https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

@luvies
Copy link
Contributor

luvies commented Jan 28, 2019

Triple slash could work, but it would have to still have the context of the import, since the normal reference directive loads the typings into the current file directly I believe, rather than how import does it (by selecting specific exports, default importing or namespace importing). To work properly, I think it would need something like

/// <reference module="..." defs="..." />

so it could then relate which definition reference mapped to which import. That, or do nearly the same thing as the // @deno-defs and have it work against the next line. The former seems very verbose, so I don't think I like it quite as much.

@kitsonk
Copy link
Contributor

kitsonk commented Jan 28, 2019

Good point - I hadn't considered that. I guess if a program is completely JS it should be able to avoid the check for d.ts and only in the situation where TS imports JS would it would look for the sibling d.ts file - that doesn't seem too bad?

So if a main module is TypeScript and a file being imported is JavaScript, Deno will look for a sibling file with the same name, but the .d.ts extension. I am not sure how easy it will be able to deal with use cases like @luvies is mentioning where you are trying to load some 3rd party JavaScript of which you don't have control of how it is served, which I think will be a common use case. The sort of "wrap with special pragma" would be explicit and cover the vast majority of use cases... sort of "here is my JS module, and here are the Type Definitions I want to use for TypeScript" instead of fumbling around on the file system trying to guess.

Just want to mention that typescript does implement something like this, special comments + imports as Triple-Slash Directives.

The TypeScript team have started to avoid the /// references in general for new features. (e.g. // @ts-nocheck, // @ts-check, and // @ts-ignore). Again, I would look for something that doesn't conflict or really couple us to expected behaviours of TypeScript. Currently TypeScript parses the <reference> and other triple slash pragma, irrespective of what we would need to do in Deno.

@ExE-Boss
Copy link

ExE-Boss commented Apr 3, 2019

Also, it might be a good idea to only search sibling directories on a local file system.

@ry ry changed the title No typing check for js files Support d.ts files Jun 9, 2019
@ry ry mentioned this issue Jun 9, 2019
43 tasks
@beshanoe
Copy link

I also don't think fetching d.ts automatically or introducing special type imports is a good idea. Kills the whole predictability and extension explicitness thing. I'd rather prefer modules merged with its types together. Since we're always dealing with ES exports, we can potentially create some tool which applies types from d.ts to the js files as JSDocs for example and publish as a separate typed package.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 12, 2019

The challenge is that many .d.ts files don't correlate 1:1 with JavaScript modules. In particular for large functional libraries, like things like lodash the absolutely don't correlate. Many many @types are "global" or "ambient" declarations, that describe a whole library. Some are written as UMD/modular types, but even then they may not correlate to TypeScript modules. So reading them and applying them to code is going to be very difficult, let alone what file it applies to.

On the other hand, you bring up an interesting potential, that if we settled on the conventions of resolving the @types for loading a JavaScript "package" off a CDN, then we could potentially try to remotely resolve the .d.ts files as remote code which we could cache. There might need to be some magic though. Hmmmm...

@oldrich-s
Copy link

Just a thought. How about to look at source map analogy - sourceMappingURL?

https://sourcemaps.info/spec.html

Multiple javascript files could point to the same d.ts file and that would then be loaded only once. It should be responsibility of the js file owner to provide a correct d.ts file. So no magic would be necessary.

@ry
Copy link
Member

ry commented Jul 22, 2019

@oldrich-s I think something like that is ideal.

Can it be achieved with triple slash directives?

/// <reference path="path/to/file.d.ts" />

@oldrich-s
Copy link

oldrich-s commented Jul 22, 2019

Yes it can but as @luvies mentions the default typescript behaviour would load the d.ts file globally and not as an ordinary import. Therefore it could make sense to follow the source map convention instead with eg //# typeMappingURL and then implement your own type injection in deno. It could be as simple as declare module "importFilePath" { ...d.ts source from typeMappingURL... }. In that case the d.ts file is required to have the same structure as the js file - with all the exports etc. So it would not be possible to bundle multiple files into one d.ts file. But again, it is the responsibility of the js file owner to assure that d.ts file corresponds to js file - the same way as h file corresponds to cpp file in c++.

Edit: actually you could support both scenarios as they do different things - regular import vs global import.

@kitsonk kitsonk mentioned this issue Jul 23, 2019
3 tasks
@kitsonk
Copy link
Contributor

kitsonk commented Aug 6, 2019

I have been thinking about this a fair amount. Proposing the following solution, which I plan to start working on. The solution needs to have the following features:

  • Be compatible with tsc and other TypeScript compilers. This means it needs to be something ignored by other runtimes where it is expected they would either resolve the types independently or simply not enforce the type checking on the external JavaScript. This also means that we can't use "real" syntax, so something like import from "something.d.ts" will not be possible.
  • Be declarative. In order to avoid "magic" lookups or complex resolution logic, applying types should be explicit, not implicit.
  • Support local and remote .d.ts files, just like we support local and remote modules.
  • Support both UMD and ambient type declarations. For UMD, this means supporting a path where each sub module is attempted to be resolved.
  • Be able to be applied "externally". Since the use case will be importing external JavaScript libraries into a Deno TypeScript programme, the declarative directives will need to be applied before the import (and therefore module resolution).

So based on these constraints/requirements, I propose the following:

  • Introduce a comment pragma of typeDefinition which will be applied to the next import statement. Specifically the following syntax: //# typeDefinition="foo.d.ts".
  • If the value ends in .d.ts it is considered either an ambient declaration file, or a UMD file applied only to that specific module. If the value ends with anything else, it will be considered a path and the following module will be resolved by using that as the root.

To make this work, I think this is what I need to build:

  • Whenever a new module is loaded by the compiler, the compiler will need to parse out the comment pragmas. When it identifies a comment pragma, it will need to parse out the next import statement.
  • The compiler will need to maintain a map of imports to pragmas.
  • When the compiler then attempts to resolve the module names, it will need to use the map to resolve to a .d.ts file instead of a JavaScript file.

I believe then that TypeScript will simply utilise the resolved .d.ts files to provide type information for the program, and not touch the underlying JavaScript. When the program is emitted, any "raw" JavaScript will be missing, but I believe as Deno starts loading the modules into runtime, any missing "raw" JavaScript will be automatically resolved and loaded.

@ry
Copy link
Member

ry commented Aug 6, 2019

@kitsonk The solution you outlined seems reasonable.

Introduce a comment pragma of typeDefinition which will be applied to the next import statement. Specifically the following syntax: //# typeDefinition="foo.d.ts".

Maybe I'm betraying my ignorance of typescript, but what about using /// <reference types="./foo.d.ts" /> ?

Whenever a new module is loaded by the compiler, the compiler will need to parse out the comment pragmas.

It would be good if this could be done as part of the checkJs compilation (i.e. actually look for it in the AST provided by TS). I suppose a regex across the file is probably not measurably slow tho - so that might work for a first pass.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 6, 2019

Maybe I'm betraying my ignorance of typescript, but what about using /// <reference types="./foo.d.ts" /> ?

That would be covered by point 1:

This means it needs to be something ignored by other runtimes where it is expected they would either resolve the types independently or simply not enforce the type checking on the external JavaScript.

The way we need to interpret the triple slash directive is different than the way tsc would need to do it, and so we would likely "break" the way it works. That is just an educated assumption at this point, one I should prove when tackling this problem, but I am pretty certain it will break.

It would be good if this could be done as part of the checkJs compilation (i.e. actually look for it in the AST provided by TS). I suppose a regex across the file is probably not measurably slow tho - so that might work for a first pass.

I thought about that, the biggest problem is that trivia (comments) are not part of the AST in TypeScript. You basically have visit the node and then query if there is trivia. Even then I would have concerns that we would have to reparse each file, because you don't get back the AST until the parse happens, which requires all imports to be resolved. So we would parse the AST, load additional .d.ts files and then have to reparse the AST. I don't think the raw, just syntax, parse is available easily.

@ExE-Boss
Copy link

ExE-Boss commented Aug 6, 2019

Actually, when using triple‑slash directives in a module, they’re used for resolving imports.

When they’re used in a script, they’re used for resolving UMD globals, which are defined on the globalThis reference only when not imported as a CommonJS or AMD module.

The exception to this are non‑module triple‑slash directives which only define ambient types, which are then available in the module or script where they’re used.

@ry
Copy link
Member

ry commented Aug 6, 2019

What about using something similar to typescript's resource directives, but something they don't use:

/// <reference denotypes="./foo.d.ts" />

Are these in the AST?

@kitsonk
Copy link
Contributor

kitsonk commented Aug 6, 2019

/// <reference denotypes="./foo.d.ts" />

There are more directives than reference and that might work, but I wouldn't want to interfere with a known directive. I would suggest something like:

/// <deno-types path="./foo.d.ts" />

Are these in the AST?

A quick check says no, they are like other trivia, related to the next nearest node, but that seems a bit strange... I need to do more research and see that while they may not be in the AST, if there is another API the surfaces up triple-slash directives. I suspect there must be.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 6, 2019

Actually I was wrong, they are in the AST, if they are "known" directives. Why I didn't see it before was using the suggested text, which doesn't get parsed. Also they aren't part of the AST, as they are attributes on the SourceFile which is the root of the AST.

So basically, anything that doesn't comply with a supported directive just looks like a comment. Also, my previous concern about when the AST would be available, and the need to "refresh" after we really resolved to model. I think the best thing to do at this point is to ensure the code is properly abstracted out, so that if we can somehow get that information from the AST in the future we would just need to change a method or two, but utilise RegEx for now (using some form of triple slash directive).

@ry
Copy link
Member

ry commented Aug 6, 2019

I think the best thing to do at this point is to ensure the code is properly abstracted out, so that if we can somehow get that information from the AST in the future we would just need to change a method or two, but utilise RegEx for now (using some form of triple slash directive).

SGTM

kitsonk added a commit to kitsonk/deno that referenced this issue Aug 7, 2019
kitsonk added a commit to kitsonk/deno that referenced this issue Aug 8, 2019
@ry ry closed this as completed in #2746 Aug 22, 2019
ry pushed a commit that referenced this issue Aug 22, 2019
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.

7 participants