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

Runtime Compiler API #3442

Merged
merged 6 commits into from
Jan 8, 2020
Merged

Runtime Compiler API #3442

merged 6 commits into from
Jan 8, 2020

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Dec 5, 2019

Fixes #2927
Fixes #3054

This PR provides 3 new Deno APIs which provide user/runtime access to the TypeScript compiler that is built into Deno. This should be a good base for creating interesting workloads on top of Deno.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Dec 5, 2019

bikeshedding: Deno.compileTS? (compile sounds more broad, even like compiling to binary, which might be a feature we support in the future)

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 5, 2019

bikeshedding: Deno.compileTS?

Well it isn't just TS, you can "compile" JavaScript and JSON with the current implementation. I think when we added the ability to register a custom compiler, that we would want to be able to just throw any of the supported media types at this and just have it return the results, irrespective of it being built in or not.

The only limitation I can see of the current API, is that when sources are provided (and they aren't resolved outside of Deno) the media type is determined by extension. There isn't a way to express a media type outside of that. An alternative version of the sources could be supported where it took an array of "source file" objects, which would allow specification of a media type, but that just increases the complexity.

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 6, 2019

Ok, the Deno.transpileOnly works now and the processing of compiler options that are passed also works. To make it functionally complete I need to do Deno.bundle. There is quite a bit of DRY that I need to address and other cleanups, as well as tests.

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 9, 2019

Ok, the PR is functionally complete now, but needs DRYing, clean-up, tests and docs.

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 11, 2019

Grrr... bundle_exports is broken and I don't know why... when I run the steps manually it actually works, but it fails as part of cargo test but I am not sure what I changed to cause it... 😡

It turned out it was a strong one, where root module specifiers sometimes included relative paths in the absolute URL (e.g. "file://foo/bar/../bar/baz.ts") but the resolved modules didn't, so the ability to determine the common root which is needed to figure out what module to instantiate in the bundle wasn't working. 🤷‍♂

@kitsonk kitsonk force-pushed the runtime_compiler branch 2 times, most recently from a2eadfb to ed065bd Compare December 13, 2019 03:55
@kitsonk kitsonk marked this pull request as ready for review December 13, 2019 04:01
@kitsonk kitsonk changed the title [WIP] Runtime Compiler API Runtime Compiler API Dec 13, 2019
@bartlomieju bartlomieju mentioned this pull request Dec 26, 2019
Comment on lines +319 to +323
* @param sources An optional key/value map of sources to be used when resolving
* modules, where the key is the module name, and the value is
* the source content. The extension of the key will determine
* the media type of the file when processing. If supplied,
* Deno will not attempt to resolve any modules externally.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API allows to programatically create TS programs programatically without writing file to disk before compilation 👍 nice

std/manual.md Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small nitpick: js/compiler_*.ts - maybe move all compiler related files under common js/compiler/ directory (or js/compilers/ to resemble layout in Rust)?

All in all I think this PR brings a lot of great functionality that users are waiting for. I'm for landing this PR, but after Tokio 0.2 PR (#3418)

@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 29, 2019

One more small nitpick: js/compiler_*.ts - maybe move all compiler related files under common js/compiler/ directory (or js/compilers/ to resemble layout in Rust)?

I thought about that, as that was my first instinct but then I remembered that @ry doesn't like sub-directories... 😁 so I was going to wait for some feedback before making that change.

Comment on lines +327 to +331
export function compile(
rootName: string,
sources?: Record<string, string>,
options?: CompilerOptions
): Promise<[Diagnostic[] | undefined, Record<string, string>]> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diagnostics are null when there are none, so what would empty array diagnostics mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you getting an empty array? I believe I coded it so that it doesn't resolve with an empty array.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why that kind of API? Empty array for no diagnostics follows naturally and is just as easy to check for. Unless this is some convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty array for no diagnostics follows naturally and is just as easy to check for.

Is it? Technically it is slightly slower, because property access is slower than a value check.

Unless this is some convention?

There are two conventions. One is the arguments to a callback convention, where err would be the first argument and undefined if there was no error. The second convention is the Rust enum Result type, where you have Err and Ok. While not directly translatable, a presence of a value would indicate that something didn't go ok.

So why would an empty array be preferable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two conventions. One is the arguments to a callback convention, where err would be the first argument and undefined if there was no error.

When there is a single potential error, the no-error case is modelled with nullability. When there is (explicitly) a list of errors, the no-error case is modelled naturally as an empty list and nullability is redundant, isn't it? null | non-empty list is generally strange to see.

My first instinct compares it more to an stderr stream which is simply empty or not (especially when it's referred to as a diagnostic).

Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.

Fixes denoland#2927
@kitsonk kitsonk force-pushed the runtime_compiler branch 2 times, most recently from 67a66f1 to 6e60373 Compare January 6, 2020 19:30
@kitsonk
Copy link
Contributor Author

kitsonk commented Jan 6, 2020

@ry rebased post rusty_v8 merge and still going strong.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kitsonk, I really apologize for taking so long to get this in. My insufficient excuses for taking so long is that w've been refactoring lower-level code and it's difficult to switch modes when thinking about that.

My concerns with this patch are:

  1. I keep thinking it would be nice to have the TypeScript compiler be less privileged and instead run as normal user code. This is clearly impossible in the near future - and it's not even clear if it would be possible under any circumstances. But it bothers me that we're unable to run the normal TypeScript compiler as user code still (like we do with, for example, prettier).
  2. I'm slightly concerned with the mismatch between subcommand names and the APIs. "Deno.bundle" matches "deno bundle" but "Deno.compile" corresponds to "deno fetch".

That said, we do have a privileged compiler facilities which deviate from TS's in functionality. Currently they are only accessible from the command-line by executing Deno - certainly it's better to be able to programmatically access these APIs, as is achieved in this patch. These APIs which you've added are all accessed through the op interface, so I think this is all good.

This patch does add a bit of runtime code, so let's watch the benchmarks for changes in snapshot size or execution time regressions.

All that said - it LGTM - it's a well written patch with tests and docs.

@ry ry merged commit d325566 into denoland:master Jan 8, 2020
@rsp rsp mentioned this pull request Jan 15, 2020
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
Also restructures the compiler TypeScript files to make them easier to
manage and eventually integrate deno_typescript fully.
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 this pull request may close these issues.

split OP_FETCH_SOURCE_FILES into two ops [Feature Request] Typescript compile API
5 participants