-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
[WIP] Add support for --types #729
Conversation
I have created and example here: https://github.com/kitsonk/deno_example using the export via
@ry the CI failure seems to be a temporary Travis environmental issue versus an issue with the PR. |
Just further to the above, I looked at the three externals. Two of them could just be appended onto the The option could be to pull the type definition in directly to the project and do the conversion at source. I do not expect a lot of churn in |
@kitsonk Nice work! I was able to try it and it works. (Also restarted the CI server and it passed.) Looking at the first couple lines of the output:
The |
@kitsonk can you give an update on the status of this? |
@ry I am still working on it, it is just slow... manipulating the AST and doing the sort of dependency analysis is a lot more complicated and while the AST is there and easy to manipulate. If we wanted to, the "ugly" working version we could commit while the elegant build is going to be a bit longer. |
@ry I have updated this PR with a rebasing of the original |
@ry I know this is taking a long time, but I am making progress. Here is the output now of https://gist.github.com/kitsonk/6c11763faf833f5cf4ec14f58657d8f8 I think I am now finally on the right path to solve this problem and deliver something that will just adjust with deno for the foreseeable future. |
@kitsonk cool - keep me updated |
f0b4bee
to
88baa9e
Compare
Ok, it took a while, but now this is functionally complete, I just need to integrate it into the build. An example of what is being generated is available here: https://gist.github.com/kitsonk/6c11763faf833f5cf4ec14f58657d8f8 |
tools/ts/build_lib.ts
Outdated
// flag, so we will just create a special `@internal` namespace and add in | ||
// just what we need "manually" | ||
const internalModule = libDTs.addNamespace({ | ||
name: `"@internal"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@internal/generated
?
tools/ts/build_lib.ts
Outdated
@@ -0,0 +1,253 @@ | |||
import { join } from "path"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be a top-level main script that depends on cwd. Because it's a lot of code, it would be good to put it inside a function with arguments as all the various inputs (e.g. cwd). This will potentially let us test it independently later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to move it to args passed from the build script, because depending on other environments it simply wouldn't work (like when the build path is outside of the root of deno like on travis) but I was just "hacking" it to work until I was ready to integrate it, which is what I am doing now.
I wonder if there's any way we can validate that the generated declaration file corresponds to the actual runtime types? |
@ry to run manually at the moment, from the root of the deno directory:
Hmmm... I wonder how easy it would be to combine into one As far as testing/validating runtime types, the basic level would be to author a |
@ry I think this is now complete and Once there is a branch for the added packages on |
tools/ts/build.js
Outdated
skipProject: true | ||
}); | ||
|
||
require("./build_lib"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this file and directory are too generic tools/ts/build.js
. Is there something more descriptive we could use?
Maybe tools/ts_declaration_builder/main.js
tools/ts/build.js
Outdated
strict: true, | ||
target: "esnext" | ||
}, | ||
skipProject: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why skipProject? Add a comment?
tools/ts/build_lib.ts
Outdated
silent = true; | ||
break; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not have top level scripts here. Can you wrap this all in a main function, and pass process.argv
to it?
tools/ts/build_lib.ts
Outdated
|
||
// Add the `compiler` module | ||
addNamespaceDeclaration( | ||
"compiler", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't the compiler module internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I researched it. See #876
tools/ts/ast_util.ts
Outdated
const MSG_GENERATED_SPECIFIER = "gen/msg_generated"; | ||
export const INTERNAL_GENERATED_SPECIFIER = "@internal/generated"; | ||
|
||
let rootPath = process.cwd(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superfluous since setRootPath()
is called elsewhere? Also please avoid top-level functionality - it would be nice if everything was wrapped in functions so we can test it.
tools/ts/ast_util.ts
Outdated
import { relative } from "path"; | ||
import { readFileSync } from "fs"; | ||
import Project, { | ||
ExportDeclaration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this syntax mean? Why is Project
not inside the braces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I've been thinking to move Project
to a named export and not have a default export (I'm the library author). It's the default export of the library right now and it's really only that way for historical reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it is supported ES6 import syntax. It is default import and then named imports. You can blame TC39 for default. Something that sounded good on paper...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project
is also a named export, I will convert to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, it isn't... DOH!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do an update that adds it as a named export tonight. I'm not a fan of default imports/exports either.
Edit: Published in 16.0.2. Seems like my declaration file was lying that it was a named export! Now it's actually exported as a named export as well and added a deprecation notice to default.
@ry I updated my branch of
So a net add of 139k SLOCs. |
We need this specific version because ts-simple-ast depends on it. See denoland#729 (comment)
We need this specific version because ts-simple-ast depends on it. See #729 (comment)
f6ef61a
to
7763135
Compare
@ry I think this is close to being ready to go, with another review and updating |
|
||
// This modules bootstraps ts-node to allow on the fly transpiling of | ||
// TypeScript while under NodeJS and performs an interpretation of | ||
// process.argv to pass to the build_library main function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to replace this with a more general tools/ts_library_builder/README.md
Let me take a stab at describing this - please modify
This tool allows us to produce a single TypeScript declaration file that describes the complete Deno runtime, including global variables and the built-in "deno" module. The output of this tool, lib.deno_runtime.d.ts, serves several purposes:
1. It is passed to the typescript compiler js/compiler.ts, so that TypeScript knows what types to expect.
2. It is outputted to stdout by `deno --types`, so that users can easily have access to the complete declaration file. Editors can use this in the future to perform type checking.
3. Because JSDocs are maintained, this serves as a simple documentation page for Deno. We will use this file to generate HTML docs in the future.
## Design
Ideally we wouldn't have to build this tool at all, and could simply use tsc to output this declaration file. However, we were not able to get it to output a clean declaration using `emitDeclarationOnly` and `outFile`.
[.... maybe add more detail here.]
## TODO
Currently the Jsdoc associated with interfaces is not properly emitted
`${basePath}/js/globals.ts` | ||
]); | ||
|
||
// TODO check diagnostics for any issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will diagnostics be printed to stdout? It would be good to have a simple assert here that fails if there are any diagnostics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The semantic, syntactic, global, options, and declaration (since declaration: true
) diagnostics are available on emitResult
below by calling emitResult.getDiagnostics()
.
Generally, diagnostics can be retrieved by calling inputProject.getPreEmitDiagnostics()
, which returns the semantic, syntactic, global, config file parsing, options, and if enabled the declaration diagnostics. For specific diagnostics, they can be retrieved from the program (ex. inputProject.getProgram().getSemanticDiagnostics()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about this and figured that actually I probably need to do this. In order to get it to work, I need to properly configure the output project so that it can resolve the other types it needs in order to be fully valid.
// actually creating it, only in memory at this stage. | ||
const libDTs = outputProject.createSourceFile(outFile); | ||
|
||
// Deal with `js/deno.ts` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider breaking this section out into its own function.
console.log(`Created module "deno".`); | ||
} | ||
|
||
// Deal with `js/globals.ts` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider breaking this section out into its own function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work - I'm ready to land this after a few things:
- Add a readme file, I outlined one above.
- Let's get all the changes to js/ landed separately (like
@internal
stuff) except for where you actually hook into lib.deno_runtime.d.ts
BUILD.gn
Outdated
# Generates the core TypeScript type library for deno that will be | ||
# included in the runtime bundle | ||
|
||
run_node("gen_runtime_lib") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to deno_runtime_declaration
BUILD.gn
Outdated
run_node("gen_declarations") { | ||
# Generates the core TypeScript type library for deno that will be | ||
# included in the runtime bundle | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove \n
args += [ | ||
"--debug", | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@ry ok, I think I have addressed all feedback. Also I am now checking diagnostics on the final There really aren't any commits in here now that make sense to be independent, unless you want to land the updates to third_party and the support for |
We need this specific version because ts-simple-ast depends on it. See denoland/deno#729 (comment)
Resolves #632
This PR allows the following:
Which will pipe the default library for deno to the console, which deno uses to validate code against internally. End users can then pipe this to a file for use in their projects and use within an editor. For example, an end user could do this:
And then configure a
tsconfig.json
with the following, which would then mirror the way deno will evaluate the code:Note I cannot think of a good way of creating a unit test for this at the JS level and it took me manually doing the export and checking the file to validate it and make sure it works. Some changes to
errors.ts
had caused some issues with the resulting types, which were only detectable that way. It might be possible to run the types throughtsc
to validate them (essentially do what is suggested above as part of the test suite) but I wanted to get feedback before pursuing that.