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 #2746

Merged
merged 2 commits into from Aug 22, 2019

Conversation

@kitsonk
Copy link
Contributor

commented Aug 7, 2019

Fixes #1432

I have a MVP where the following is working:

/// <deno-types path="./foo.d.ts" />
import * as foo from "./foo.js";

/// <deno-types path="./bar.d.ts" />
import * as bar from "./bar.js";

console.log(foo.foo, bar.bar);

Where instead of TypeScript trying to load foo.js and bar.js, it instead loads foo.d.ts and bar.d.ts. There are quite a few more scenarios that need to be supported and I need to try it out with some real world code, but it looks like it will work.

@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

Awesome - I'm excited about this - not being able to use the TypeScript compiler in our TypeScript runtime has been very annoying.

Just a quick atheistic comment:

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

I actually preferred your original //# typeDefinition="./bar.d.ts" but suggested triple-slash reference directive as a way of potentially being able to extract the information from the typescript AST. If /// <reference ...something.../> isn't the syntax we use, might as well use the //# syntax, which has roots in web standards.

Shortening //# typeDefinition= to //# types= could be done without introducing confusion, I think.

tests/type_definitions/main.ts Outdated Show resolved Hide resolved
@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Ok, so my working branch, this now works:

// @deno-types="https://unpkg.com/dayjs@1.8.15/index.d.ts"
import dayjs from "https://unpkg.com/dayjs@1.8.15/esm/index.js";

const date = dayjs();
console.log(date.seconds());

Which has an error (the API is second(), not seconds()), so it now outputs:

Compile file:///Users/kkelly/github/deno/tests/type_definitions/remote/main.ts
Download https://unpkg.com/dayjs@1.8.15/index.d.ts
error TS2551: Property 'seconds' does not exist on type 'Dayjs'. Did you mean 'second'?

► file:///Users/kkelly/github/deno/tests/type_definitions/remote/main.ts:5:18

5 console.log(date.seconds());
                   ~~~~~~~

  'second' is declared here.

    ► https://unpkg.com/dayjs@1.8.15/index.d.ts:46:5

    46     second(): number
           ~~~~~~

Fixing it then outputs:

Compile file:///Users/kkelly/github/deno/tests/type_definitions/remote/main.ts
Download https://unpkg.com/dayjs@1.8.15/index.d.ts
Download https://unpkg.com/dayjs@1.8.15/esm/index.js
Download https://unpkg.com/dayjs@1.8.15/esm/constant
Download https://unpkg.com/dayjs@1.8.15/esm/utils
Download https://unpkg.com/dayjs@1.8.15/esm/locale/en
Download https://unpkg.com/dayjs@1.8.15/esm/constant.js
Download https://unpkg.com/dayjs@1.8.15/esm/utils.js
Download https://unpkg.com/dayjs@1.8.15/esm/locale/en.js
32

Getting excited... The other part I need to do though is that if the value of the directive looks like a directory to resolve the .d.ts files relative to that directory, in order to support UMD type definitions (where each module has its own .d.ts file).

@kitsonk kitsonk force-pushed the kitsonk:support-dts branch from deb492a to 606fe2a Aug 8, 2019

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Ok, I have tested now with lodash-es but using the types from @types/lodash and that appears to be all working:

// @deno-types="https://unpkg.com/@types/lodash@4.14.136/index.d.ts"
import * as _ from "https://unpkg.com/lodash-es@4.17.15/lodash.js";

console.log(_.add(1, 2));

The change I made is that the hint only applies to the next import/export statement. My original thought would be that we might have several imports in a row and that we would resolve the type definition for each on based on a path, but that doesn't make a lot of sense in practice.

We still won't be able to support all type definitions at the moment, since some try to import or reference types from other type libraries. (e.g. /// <reference types="node" />). Because we don't magically resolve them, we don't know where to look. Right now they simply get passed upstream and Rust complains about not being able to resolve the module, but provides a cryptic message. I want to catch those earlier and have something more meaningful.

@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

Are you able to load the TS compiler with this? (Review soon...)

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Are you able to load the TS compiler with this?

That is my next use case to test.

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@ry so it works from this PRs perspective. The following:

// @deno-types="https://unpkg.com/typescript@3.5.3/lib/typescript.d.ts";
import * as ts from "https://unpkg.com/typescript@3.5.3/lib/typescript.js";

console.log(ts.version);

Outputs:

$ deno --reload tests/type_definitions/typescript/main.ts
Compile file:///Users/kkelly/github/deno/tests/type_definitions/typescript/main.ts
Download https://unpkg.com/typescript@3.5.3/lib/typescript.d.ts
error TS2551: Property 'versions' does not exist on type 'typeof ts'. Did you mean 'version'?

► file:///Users/kkelly/github/deno/tests/type_definitions/typescript/main.ts:4:16

4 console.log(ts.versions);
                 ~~~~~~~~

  'version' is declared here.

    ► https://unpkg.com/typescript@3.5.3/lib/typescript.d.ts:19:11

    19     const version: string;
                 ~~~~~~~

But the way the typescript.js is formed, nothing gets exported as the module, so Deno loads it, but we get an empty object returned back. Looking at the script, I am a little confused how it works at all, under node or rollup. It is a global script that creates a var ts variable (plus a couple others) and runs it through a series of IIFE. I am not sure how it "exports" anything. 😕

@afinch7

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

If you look at the end of https://raw.githubusercontent.com/microsoft/TypeScript/master/lib/typescript.js
You can see this commonjs export

    if (typeof module !== "undefined" && module.exports) {
        module.exports = ts;
    }

not sure if this is the only export. I did manage to get typescript imported with .d.ts types after creating a rollup bundle that just reexports everything.

@kitsonk kitsonk force-pushed the kitsonk:support-dts branch from 606fe2a to a6ffa8c Aug 17, 2019

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@afinch7 that makes sense, and I don't know how I missed it. Currently we don't have an easy way to load stuff like this in Deno (nor do I think we should). With known things like TypeScript, it should be fairly easy to wrap it and have it be a ESM file. But in theory it is tangental to this PR. So we can now load the types, but we can't load the module.

Since really TypeScript should support output to ESM, I opened issue microsoft/TypeScript#32949 to request it.

@kitsonk kitsonk force-pushed the kitsonk:support-dts branch from a6ffa8c to 51b93da Aug 17, 2019

@kitsonk kitsonk marked this pull request as ready for review Aug 17, 2019

@kitsonk kitsonk changed the title [WIP] Support .d.ts files Support .d.ts files Aug 17, 2019

@kitsonk kitsonk referenced this pull request Aug 17, 2019
3 of 3 tasks complete

The TypeScript compiler supports triple-slash directives, including a type
reference directive. If Deno used this, it would interfere with the behavior of
the TypeScript compiler.

This comment has been minimized.

Copy link
@ry

ry Aug 19, 2019

Collaborator

We could maybe intercept and remove a triple slash reference...

I'm still not sure about this syntax.

This comment has been minimized.

Copy link
@kitsonk

kitsonk Aug 20, 2019

Author Contributor

If we used something that conflicted with TypeScript, we could remove it for Deno, but we would want it to be a noop everywhere else, otherwise we would have Deno only TypeScript.

Even for Deno, if it overlapped TypeScript syntax, we would need a way to disambiguate it, which I don't think would be easy, so we could use a triple-slash directive, but it would have to be something TypeScript currently ignores, so the following would be out:

/// <reference path="..." />
/// <reference types="..." />
/// <reference lib="..." />
/// <reference no-default-lib="true"/>

So something like this "could" work and would be terse enough:

/// <reference deno="..." />

The biggest "challenge" I see is that these are like include statements, and they effect the file they are in. What we need to support in Deno is something where certain statements are impacted, so they are external and their position matters. Because there is such a dramatic behaviour change, it maybe confusing.

@ry
ry approved these changes Aug 22, 2019
Copy link
Collaborator

left a comment

LGTM - sorry for the delay. This is an important feature, thanks @kitsonk.

I have concerns about this syntax, but it's better to merge this and edit than risk conflicts.

@ry ry merged commit 6c7d337 into denoland:master Aug 22, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

This patch seems to have reduced the thread count for cold_relative_import from 13 to 11. That pleasant but surprising.

Screen Shot 2019-08-22 at 12 29 00 PM

@kitsonk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Thanks. Yeah I think it is best to iterate on it, even if we change the syntax. Hard to tell what we don't like. It may also struggle with some real world use cases which we can start to address.

I implemented a straight forward cache in the compiler that existed previously. I did it to help ensure once we had replaced a file we knew it. I suspect that had a side impact of reducing the threads that went back to Rust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.