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: untangle cli/compiler.rs and cli/deno_dir.rs #2636

Merged
merged 61 commits into from Jul 17, 2019

Conversation

@bartlomieju
Copy link
Contributor

commented Jul 11, 2019

More preliminary work for #2057

This PR untangles cli/compiler.rs and cli/deno_dir.rs.

  • use version hash to decide if compilation is needed (base on work of @kevinkassimo in #2311)
  • remove fetching of compiled source code from DenoDir.fetch_module_meta_data
  • refactor ModuleMetaData:
    • rename to SourceFile
    • remove TS specific fields (maybe_output_code, maybe_source_map)
  • abstract TS compiler as TsCompiler in cli/compiler.rs
  • move implementation of SourceMapGetter to cli/compiler.rs
  • DiskCache
  • add SourceFileCache to prevent multiple disk fetches of source files
  • update DenoDir and TsCompiler to use DiskCache API

TODO:

  • cleanup of compiler pipeline
@bartlomieju
Copy link
Contributor Author

left a comment

This is starting to come together, @ry PTAL when you have a moment. There are still a lot of stuff that should be moved from deno_dir.rs to compiler.rs but you should see my vision now

cli/compiler.rs Show resolved Hide resolved
cli/deno_dir.rs Outdated Show resolved Hide resolved
cli/deno_dir.rs Outdated Show resolved Hide resolved
cli/main.rs Outdated Show resolved Hide resolved
cli/ops.rs Outdated Show resolved Hide resolved

@bartlomieju bartlomieju force-pushed the bartlomieju:refactor_compiler_deno_dir branch from d97cd3f to 9687dd2 Jul 12, 2019

cli/compiler.rs Outdated Show resolved Hide resolved
pub fn compile_async(
self: &Self,
state: ThreadSafeState,
source_file: &SourceFile,

This comment has been minimized.

Copy link
@ry

ry Jul 17, 2019

Collaborator

It's odd that this function takes a source_file argument rather than a URL.

This comment has been minimized.

Copy link
@ry

ry Jul 17, 2019

Collaborator

Or should it be specifier: &ModuleSpecifier ?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 17, 2019

Author Contributor

Yeah I still haven't settled on API... A few lines below there's TODO about fetching SourceFile inside compiler_async. I'll leave it as is, and figure out compiler pipeline in followup PR

cli/compiler.rs Show resolved Hide resolved
}

impl SourceFile {
// TODO: this method should be implemented on CompiledSourceFile trait

This comment has been minimized.

Copy link
@ry

ry Jul 17, 2019

Collaborator

I don't see CompiledSourceFile anywhere here. Is the TODO saying that a new trait called CompiledSourceFile should be created with js_source() method?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 17, 2019

Author Contributor

More-or-less, more info in #2636 (comment)


// TODO: this should be removed, but integration test 022_info_flag depends on
// using "/" (forward slashes) or doesn't match output on Windows

This comment has been minimized.

Copy link
@ry

ry Jul 17, 2019

Collaborator

Ok, let's do this in a follow up PR. We can just extend the wildcard in the first line of tests/022_info_flag.out

This comment has been minimized.

Copy link
@ry

ry Jul 18, 2019

Collaborator
@ry
Copy link
Collaborator

left a comment

This refactor cleans up a lot of things - really nice work. There are of course many loose ends, but that's inevitable. We'll have some clean up PRs after this - but we should get this in sooner rather than later.

I'm good to land once @piscisaureus approves.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

This refactor cleans up a lot of things - really nice work. There are of course many loose ends, but that's inevitable. We'll have some clean up PRs after this - but we should get this in sooner rather than later.

Thanks! Unfortunately there are, but I truly hope it will make everything more approachable to allow us to move forward easily.

I'm good to land once @piscisaureus approves.

👍

bartlomieju added some commits Jul 17, 2019

}

debug!("is remote but didn't find module");
/// Asynchronously fetch remote source file specified by the URL following redirects.
fn fetch_remote_source_async(

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 17, 2019

Author Contributor

There's one quirk about this method regarding redirects: we only save headers.json file for requested URL and one for final URL after all redirects.

Consider such scenario (with tools/http_server.py running):

download http://localhost:4548/tests/001_hello.js
redirect to http://localhost:4546/tests/001_hello.js 
redirect to http://localhost:4545/tests/001_hello.js 
fetch from http://localhost:4545/tests/001_hello.js 
SourceFile(url: "http://localhost:4545/tests/001_hello.js", redirect_source_url: "http://localhost:4548/tests/001_hello.js")
cache headers for "http://localhost:4548/tests/001_hello.js": redirect -> http://localhost:4546/tests/001_hello.js
cache file "http://localhost:4546/tests/001_hello.js"
cache headers for "http://localhost:4546/tests/001_hello.js"
download http://localhost:4546/tests/001_hello.js
redirect to http://localhost:4545/tests/001_hello.js 
fetch from http://localhost:4545/tests/001_hello.js  // !!! unnecessary fetch, we already have cached file "http://localhost:4545/tests/001_hello.js"
SourceFile(url: "http://localhost:4545/tests/001_hello.js", redirect_source_url: "http://localhost:4546/tests/001_hello.js")

Should we save headers for all intermediate redirects 🤔 ?

CC @ry

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Jul 17, 2019

Collaborator

Should we save headers for all intermediate redirects 🤔 ?

Yes

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

I approve too

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

I'll get CI green and start creating issues tomorrow for follow up stuff

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

Bartek - sounds good. Please draft a commit message too. Thanks!

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Rewrite DenoDir

* rename `ModuleMetaData` to `SourceFile` and remove TS specific functionality

* add `TsCompiler` struct encapsulating processing of TypeScript files

* move `SourceMapGetter` trait implementation to `//cli/compiler.rs`

* add low-level `DiskCache` API for general purpose caches and use it in `DenoDir` and `TsCompiler` for filesystem access

* don't use hash-like filenames for compiled modules, instead use metadata file for storing compilation hash

* add `SourceFileCache` for in-process caching of loaded files for fast subsequent access

* define `SourceFileFetcher` trait encapsulating loading of local and remote files and implement it for `DenoDir`

* define `use_cache` and `no_fetch` flags on `DenoDir` instead of using in fetch methods

@bartlomieju bartlomieju force-pushed the bartlomieju:refactor_compiler_deno_dir branch from d880f08 to 51c98da Jul 17, 2019

@ry

ry approved these changes Jul 17, 2019

Copy link
Collaborator

left a comment

LGTM!

@ry ry merged commit 8214b68 into denoland:master Jul 17, 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:refactor_compiler_deno_dir branch Jul 18, 2019

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