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

refactor: cleanup compiler pipeline #2686

Merged
merged 3 commits into from Jul 31, 2019

Conversation

@bartlomieju
Copy link
Contributor

commented Jul 27, 2019

Closes #2657 and #2658

Depends on #2683

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Okay I addressed all the things I wanted to in this PR.

Brief summary:

  • remove fetch_source_file_and_maybe_compile_async and replace it with State.fetch_compiled_module
  • introduce CompiledModule which is basically the same as deno::SourceInfo and represents arbitrary file that has been compiled to JS module
  • introduce //cli/compilers module containing all compilers
  • introduce JsCompiler which is "identity" compiler - output is the same as input, no compilation takes place - it is used for MediaType::JavaScript and MediaType::Unknown
  • introduce JsonCompiler that wraps JSON in default export (previously it was done by SourceFile.js_source())
  • JS-to-JS compilation using checkJs

Implemented solution allows to handle other media types rather easily:

  1. Add another media type to msg::MediaType
  2. Create compiler structure implementing compile_async method that takes raw source file and produces compiled JS module out of it
  3. Wire the compiler in State.fetch_compiled_module

This PR is still missing tests, but I first want to gather feedback regarding this refactor before doing that.

CC @ry @piscisaureus @kitsonk @kevinkassimo

Show resolved Hide resolved cli/compiler.rs Outdated
// If `checkJs` is set to true in `compilerOptions` then we're gonna be compiling
// JavaScript files as well
let config_json = config.json()?;
let compile_js = match &config_json.get("compilerOptions") {

This comment has been minimized.

Copy link
@ry

ry Jul 30, 2019

Collaborator

Is the config parsed elsewhere in rust land ? If so maybe it makes sense to cache the parsed json object?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 30, 2019

Author Contributor

No, that's the only place in Rust

This comment has been minimized.

Copy link
@kitsonk

kitsonk Jul 30, 2019

Contributor

Just a health warning, tsconfig.json supports JSONC, which serde does not support. This means that it will not be able to parse certain valid tsconfig.json files. When you scaffold a tsconfig.json via the tsc it includes comments, and so I think it will be a pretty common pattern in the wild to use JSONC. So detecting checkJs may need to be done in another way, likely a regular expression of some sort. It also might make sense to have the test fixtures have a tsconfig.json that includes comments.


pub struct JsCompiler {}

impl JsCompiler {

This comment has been minimized.

Copy link
@ry

ry Jul 30, 2019

Collaborator

The idea is that in the future these will be trait impl ?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 30, 2019

Author Contributor

Yes, exactly. I'll try to refactor TsCompiler not to require state arg during compilation and factor out Compiler trait

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 31, 2019

Author Contributor

This gonna need some more thought... We're updating metrics on state when spinning up TS compiler so ATM it looks like state is a required argument. I'm gonna leave it as is now and addresses in follow up PR.

@bartlomieju bartlomieju force-pushed the bartlomieju:cleanup_compiler_pipeline branch 2 times, most recently from 419be63 to 082c4a0 Jul 31, 2019

refactor: cleanup compilation pipeline
* remove fetch_source_file_and_maybe_compile_async and replace it with State.fetch_compiled_module

* remove SourceFile.js_source()

* introduce CompiledModule which is basically the same as deno::SourceInfo and represents arbitrary file that has been compiled to JS module

* introduce //cli/compilers module containing all compilers

* introduce JsCompiler which is a noop compiler
  - output is the same as input, no compilation takes place
  - it is used for MediaType::JavaScript and MediaType::Unknown

* introduce JsonCompiler that wraps JSON in default export

* support JS-to-JS compilation using checkJs

@bartlomieju bartlomieju force-pushed the bartlomieju:cleanup_compiler_pipeline branch from 082c4a0 to 232d748 Jul 31, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Ready for review @ry

pub struct JsCompiler {}

impl JsCompiler {
pub fn compile_async(

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

All of these compile_async methods don't seem to do anything async. It's passed a source_file, which already contains the data. It seems to just wrap it up in a FutureResult... is there a more appropriate name for this method?

@@ -290,22 +329,12 @@ impl TsCompiler {
self: &Self,

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

Oops - I see that the real "compile_async" is here.. ok ignore my previous comment.

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2019

Is there a test showing support JS-to-JS compilation using checkJs? It doesn't seem so..

Show resolved Hide resolved cli/compilers/mod.rs Outdated
Show resolved Hide resolved cli/compilers/json.rs
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Is there a test showing support JS-to-JS compilation using checkJs? It doesn't seem so..

@ry addressed feedback and added tests/038_checkjs.test which ensures that JS files are run through TS compiler when checkJs is on

@@ -0,0 +1,6 @@
// console.log intentionally misspelled to trigger a type error
consol.log("hello world!");

This comment has been minimized.

Copy link
@ry

ry Jul 31, 2019

Collaborator

👍 Thanks

@ry

ry approved these changes Jul 31, 2019

Copy link
Collaborator

left a comment

LGTM

@ry ry merged commit 2e1ab82 into denoland:master Jul 31, 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

@bartlomieju bartlomieju deleted the bartlomieju:cleanup_compiler_pipeline branch Jul 31, 2019

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.