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.

Sign up for GitHub

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

Jump to bottom

fix(lsp): regression - formatting was broken on windows #21972

Merged
merged 5 commits into from
Jan 18, 2024
Merged

fix(lsp): regression - formatting was broken on windows #21972

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e19537d
fix(lsp): regression - formatting was broken on windows
dsherret Jan 17, 2024
18aa132
Fix tests.
dsherret Jan 17, 2024
92700da
Update deno_config
dsherret Jan 17, 2024
584320f
Update
dsherret Jan 17, 2024
39b8028
Clippy
dsherret Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions cli/args/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ pub mod package_json;
pub use self::import_map::resolve_import_map_from_specifier;
use self::package_json::PackageJsonDeps;
use ::import_map::ImportMap;
use deno_config::glob::PathOrPattern;
use deno_core::resolve_url_or_path;
use deno_npm::resolution::ValidSerializedNpmResolutionSnapshot;
use deno_npm::NpmSystemInfo;
Expand Down Expand Up @@ -1666,15 +1665,7 @@ fn resolve_files(
}
}
Ok(FilePatterns {
include: {
let files = match maybe_files_config.include {
Some(include) => include,
None => PathOrPatternSet::new(vec![PathOrPattern::Path(
initial_cwd.to_path_buf(),
)]),
};
Some(files)
},
include: maybe_files_config.include,
Copy link
Member Author

@dsherret dsherret Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This reverts this change I made. It shouldn't have been done from what I can tell atm

Edit: Tests failed on CI, so maybe not. Edit 2: Oh, it was used to get the base path. Edit 3: denoland/deno_config#31

exclude: maybe_files_config.exclude,
})
}
Expand Down
21 changes: 12 additions & 9 deletions cli/lsp/language_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use base64::Engine;
use deno_ast::MediaType;
use deno_core::anyhow::anyhow;
use deno_core::anyhow::Context;
use deno_core::error::AnyError;
use deno_core::parking_lot::Mutex;
use deno_core::resolve_url;
Expand All @@ -29,6 +28,7 @@ use std::collections::HashMap;
use std::collections::HashSet;
use std::env;
use std::fmt::Write as _;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;
use tokio::sync::mpsc::unbounded_channel;
Expand Down Expand Up @@ -237,6 +237,7 @@ pub struct Inner {
/// The collection of documents that the server is currently handling, either
/// on disk or "open" within the client.
pub documents: Documents,
initial_cwd: PathBuf,
http_client: Arc<HttpClient>,
task_queue: LanguageServerTaskQueue,
/// Handles module registries, which allow discovery of modules
Expand Down Expand Up @@ -527,6 +528,9 @@ impl Inner {
diagnostics_state.clone(),
);
let assets = Assets::new(ts_server.clone());
let initial_cwd = std::env::current_dir().unwrap_or_else(|_| {
panic!("Could not resolve current working directory")
});

Self {
assets,
Expand All @@ -538,6 +542,7 @@ impl Inner {
diagnostics_server,
documents,
http_client,
initial_cwd,
maybe_global_cache_path: None,
maybe_import_map: None,
maybe_import_map_uri: None,
Expand Down Expand Up @@ -874,6 +879,7 @@ impl Inner {

let npm_resolver = create_npm_resolver(
&deno_dir,
&self.initial_cwd,
&self.http_client,
self.config.maybe_config_file(),
self.config.maybe_lockfile(),
Expand Down Expand Up @@ -1046,20 +1052,18 @@ impl Inner {
self.fmt_options = Default::default();
self.lint_options = Default::default();
if let Some(config_file) = self.get_config_file()? {
// this doesn't need to be an actual directory because flags is specified as `None`
Copy link
Member Author

@dsherret dsherret Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Lies I wrote.

let dummy_args_cwd = PathBuf::from("/");
let lint_options = config_file
.to_lint_config()
.and_then(|maybe_lint_config| {
LintOptions::resolve(maybe_lint_config, None, &dummy_args_cwd)
LintOptions::resolve(maybe_lint_config, None, &self.initial_cwd)
})
.map_err(|err| {
anyhow!("Unable to update lint configuration: {:?}", err)
})?;
let fmt_options = config_file
.to_fmt_config()
.and_then(|maybe_fmt_config| {
FmtOptions::resolve(maybe_fmt_config, None, &dummy_args_cwd)
FmtOptions::resolve(maybe_fmt_config, None, &self.initial_cwd)
})
.map_err(|err| {
anyhow!("Unable to update formatter configuration: {:?}", err)
Expand Down Expand Up @@ -1148,6 +1152,7 @@ impl Inner {

async fn create_npm_resolver(
deno_dir: &DenoDir,
initial_cwd: &Path,
http_client: &Arc<HttpClient>,
maybe_config_file: Option<&ConfigFile>,
maybe_lockfile: Option<&Arc<Mutex<Lockfile>>>,
Expand All @@ -1161,9 +1166,7 @@ async fn create_npm_resolver(
create_cli_npm_resolver_for_lsp(if is_byonm {
CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions {
fs: Arc::new(deno_fs::RealFs),
root_node_modules_dir: std::env::current_dir()
.unwrap()
.join("node_modules"),
root_node_modules_dir: initial_cwd.join("node_modules"),
})
} else {
CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions {
Expand Down Expand Up @@ -3692,7 +3695,7 @@ impl Inner {
type_check_mode: crate::args::TypeCheckMode::Local,
..Default::default()
},
std::env::current_dir().with_context(|| "Failed getting cwd.")?,
self.initial_cwd.clone(),
self.config.maybe_config_file().cloned(),
self.config.maybe_lockfile().cloned(),
self.maybe_package_json.clone(),
Expand Down
20 changes: 12 additions & 8 deletions cli/tests/integration/lsp_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8507,13 +8507,15 @@ fn lsp_format_exclude_default_config() {
#[test]
fn lsp_format_json() {
let context = TestContextBuilder::new().use_temp_cwd().build();
let temp_dir_path = context.temp_dir().path();
// Also test out using a non-json file extension here.
// What should matter is the language identifier.
let lock_file_path = temp_dir_path.join("file.lock");
let mut client = context.new_lsp_command().build();
client.initialize_default();
client.did_open(json!({
"textDocument": {
// Also test out using a non-json file extension here.
// What should matter is the language identifier.
"uri": "file:///a/file.lock",
"uri": lock_file_path.uri_file(),
"languageId": "json",
"version": 1,
"text": "{\"key\":\"value\"}"
Expand All @@ -8524,7 +8526,7 @@ fn lsp_format_json() {
"textDocument/formatting",
json!({
"textDocument": {
"uri": "file:///a/file.lock"
"uri": lock_file_path.uri_file(),
},
"options": {
"tabSize": 2,
Expand Down Expand Up @@ -8635,11 +8637,12 @@ fn lsp_json_import_with_query_string() {
#[test]
fn lsp_format_markdown() {
let context = TestContextBuilder::new().use_temp_cwd().build();
let markdown_file = context.temp_dir().path().join("file.md");
let mut client = context.new_lsp_command().build();
client.initialize_default();
client.did_open(json!({
"textDocument": {
"uri": "file:///a/file.md",
"uri": markdown_file.uri_file(),
"languageId": "markdown",
"version": 1,
"text": "# Hello World"
Expand All @@ -8650,7 +8653,7 @@ fn lsp_format_markdown() {
"textDocument/formatting",
json!({
"textDocument": {
"uri": "file:///a/file.md"
"uri": markdown_file.uri_file()
},
"options": {
"tabSize": 2,
Expand Down Expand Up @@ -8705,11 +8708,12 @@ fn lsp_format_with_config() {
builder.set_config("./deno.fmt.jsonc");
});

let ts_file = temp_dir.path().join("file.ts");
client
.did_open(
json!({
"textDocument": {
"uri": "file:///a/file.ts",
"uri": ts_file.uri_file(),
"languageId": "typescript",
"version": 1,
"text": "export async function someVeryLongFunctionName() {\nconst response = fetch(\"http://localhost:4545/some/non/existent/path.json\");\nconsole.log(response.text());\nconsole.log(\"finished!\")\n}"
Expand All @@ -8722,7 +8726,7 @@ fn lsp_format_with_config() {
"textDocument/formatting",
json!({
"textDocument": {
"uri": "file:///a/file.ts"
"uri": ts_file.uri_file()
},
"options": {
"tabSize": 2,
Expand Down
Loading