fix(check): compiler options from workspace members #27009
Conversation
|
Yeah this is not for landing yet |
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
nayeemrmn
changed the title
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
…ber-compiler-options
| # TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_graph/pull/565 lands! | ||
| [patch.crates-io] | ||
| deno_graph = { git = "https://github.com/denoland/deno_graph.git", rev = "70ca7ebbb7fe9bb6cf35115ef9531aa58aadec55" } |
There was a problem hiding this comment.
To address before landing
| impl CliFactoryWithWorkspaceFiles { | ||
| #[allow(clippy::type_complexity)] | ||
| pub async fn from_flags( | ||
| flags: Arc<Flags>, | ||
| get_dirs_with_files: impl FnOnce( | ||
| &CliOptions, | ||
| ) -> Result< | ||
| Vec<(Arc<WorkspaceDirectory>, FilePatterns)>, | ||
| AnyError, | ||
| >, | ||
| collect_specifiers: impl Fn( | ||
| FilePatterns, | ||
| Arc<CliOptions>, | ||
| Arc<CliFileFetcher>, | ||
| ) -> std::pin::Pin< | ||
| Box< | ||
| dyn Future< | ||
| Output = Result<Vec<(ModuleSpecifier, SpecifierInfo)>, AnyError>, | ||
| >, | ||
| >, | ||
| >, | ||
| extract_doc_files: Option<fn(File) -> Result<Vec<File>, AnyError>>, | ||
| watcher_communicator: Option<&Arc<WatcherCommunicator>>, | ||
| ) -> Result<Self, AnyError> { |
There was a problem hiding this comment.
This API serves to hide the currently unintuitive setup sequence of creating a CliOptions, using that get a list of member dirs, patching the CliOptions with them and creating a CliFactory from that. I think it nicely shows what logic is and isn't shared between subcommands at the moment.
There was a problem hiding this comment.
I still don't get why all these changes are necessary. I'm going to take a look more at the code locally so I can understand what's going on and why this PR isn't just a change in TypeChecker. I made a pretty big change to how deno resolution gets created in #27766, so don't worry about merging that PR. I can handle the conflicts in either that PR or this one.
There was a problem hiding this comment.
There's a tangled relationship between interpreting the file arguments and factory creation (use file arguments to understand what config scopes to initialise the factory with, and then use the factory to expand them into specifiers). So I wanted a struct that jointly creates and stores these.
I also refactored the test and bench specifier collection so they could be deduped here.
There was a problem hiding this comment.
I investigated this and I think we don't need to make such foundational changes to do this change. I'm going to open a separate PR that builds off this one so we can discuss further. I think we can make this quite targeted to only TypeChecker.
There was a problem hiding this comment.
Why would we have different behaviour when using a --config flag? Shouldn't it be the same because it discovers the workspace regardless?
There was a problem hiding this comment.
I made the behaviour so that if you pass -c it will only find that config file (and its root if it's not a root itself) and treat everything like it's in that scope.
For example if you have deno.json, member_a/deno.json and member_b/deno.json but you pass -c member_a/deno.json, all files will use { root: "deno.json", member: "member_a/deno.json"}`.
I might have broke this since the last review since there's more reliance on start_dir...
There was a problem hiding this comment.
I'm not sure we should do that because it differs from other behaviour. In other parts of the CLI it will have the same behaviour no matter which config file in a workspace you provide.
| @@ -0,0 +1,8 @@ | |||
| // We shouldn't get diagnostics from this import under this check scope. | |||
There was a problem hiding this comment.
Unlike the LSP, we actually do want to see diagnostics under different scopes because they might contribute to errors seen in the current scope. I've updated the next PR to handle this and group/display the diagnostics by config.
|
Continuing in #27785. |
Closes #24504.
Blocked by denoland/deno_config#143.
Blocked by denoland/deno_graph#565.