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: new module graph used for no check #7621

Merged
merged 6 commits into from
Sep 24, 2020

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Sep 22, 2020

This PR is part of #7225 and introduces a new module graph that is more fit for purpose for how Deno handles code and dependencies. Because the refactor will take a while to do and we don't want huge PR, the first step is to utilise the module graph to do the --no-check transpile only.

This makes the whole API far more easier to understand what is going on, and makes it overall more maintainable. A simplified version of doing the transpile now looks like this:

let handler =
  Rc::new(RefCell::new(FetchHandler::new(&flags, &permissions)?));
let mut builder = GraphBuilder::new(handler, maybe_import_map);
builder.insert(&module_specifier).await?;
let mut graph = builder.get_graph();

let maybe_config = get_maybe_config();

let (stats, maybe_ignored_options) =
  graph.transpile(TranspileOptions {
    debug: flags.log_level == Some(log::Level::Debug),
    maybe_config,
  })?;

debug!("{}", stats);
if let Some(ignored_options) = maybe_ignored_options {
  println!("Some compiler options were ignored:\n  {}", ignored_options);
}

The module graph becomes the "controller" which orchestrates fetching modules, doing dependency analysis, and reading and writing to the disk cache.

Things to do on this PR:

  • Moar tests
  • Remove unused code

@kitsonk kitsonk force-pushed the module_graph_refactor branch from bfeb062 to 93bef65 Compare September 22, 2020 10:49
@ry
Copy link
Member

ry commented Sep 22, 2020

Does this mean there will be two module graphs? This patch is a rather large net addition of code, so it needs to be carefully considered before landing.

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 22, 2020

Yes, two module graph for a small period of time as we transition, as a total refactor was considered a bad idea. I tried other ways of swallowing the elephant, and this was the best I could come up with. Expand and then contract...

At the end of the journey, it will result in thousands of lines of less code.

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 22, 2020

Just to give an idea of the long term roadmap, is something like this:

  • tsc.rs should go from ~1700 > ~600
  • module_graph.rs would be fully replaced (~970)
  • a reduction of ~300-~400 in file_fetcher.rs
  • the JavaScript code in tsc should go from ~1800 down to ~430

I suspect I need to add about ~500 lines to the new graph.rs to facilitate the rest of it. So in the end a savings of about 3k of code.

@kitsonk kitsonk changed the title [WIP] refactor: new module graph used for no check refactor: new module graph used for no check Sep 23, 2020
@kitsonk kitsonk marked this pull request as ready for review September 23, 2020 02:43
@kitsonk kitsonk requested review from bartlomieju and ry September 23, 2020 02:44
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.

LGTM! Just a few minor comments. I understand that this is transitive state before complete cleanup so I don't mind additional code for a few days.

cli/global_state.rs Show resolved Hide resolved
cli/global_state.rs Show resolved Hide resolved
}

#[derive(Clone, Debug, PartialEq)]
pub struct Stats(Vec<(String, u128)>);
Copy link
Member

Choose a reason for hiding this comment

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

Use type alias instead? That way there will be no need for custom Deserialize implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a custom fmt:Display as well. It is nice to have it all be self contained, IMO.

cli/graph.rs Outdated Show resolved Hide resolved
@kitsonk kitsonk force-pushed the module_graph_refactor branch from 5a851d2 to ea466d2 Compare September 24, 2020 11:17
@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 24, 2020

@bartlomieju addressed your feedback, PTAL.

@bartlomieju
Copy link
Member

@kitsonk LGTM

@kitsonk kitsonk merged commit c489589 into denoland:master Sep 24, 2020
@kitsonk kitsonk mentioned this pull request Sep 25, 2020
13 tasks
dsherret added a commit that referenced this pull request Jun 9, 2023
)

I'm unsure why we canonicalize the config file path when loading and the
canonicalization is causing issues in #19431 because everything in the
lsp is not canonicalized except the config file (actually, the config
file is only canonicalized when auto-discovered and not whens pecified).
We also don't canonicalize module paths when loading them.

Canonicalization was added in #7621
bartlomieju pushed a commit that referenced this pull request Jun 15, 2023
)

I'm unsure why we canonicalize the config file path when loading and the
canonicalization is causing issues in #19431 because everything in the
lsp is not canonicalized except the config file (actually, the config
file is only canonicalized when auto-discovered and not whens pecified).
We also don't canonicalize module paths when loading them.

Canonicalization was added in #7621
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.

3 participants