From 3db5fd2724ba3f4986b3288bad2519e806adb0c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Feb 2023 16:25:24 -0500 Subject: [PATCH 1/4] fix(npm): allow resolving from package.json when an import map exists --- cli/resolver.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/cli/resolver.rs b/cli/resolver.rs index db6ef0c8eb9a1c..bcc1c8da8ab087 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -113,22 +113,30 @@ impl Resolver for CliGraphResolver { specifier: &str, referrer: &ModuleSpecifier, ) -> Result { - if let Some(import_map) = &self.maybe_import_map { - return import_map - .resolve(specifier, referrer) - .map_err(|err| err.into()); - } + // attempt to resolve with the import map first + let maybe_import_map_err = match self + .maybe_import_map + .as_ref() + .map(|import_map| import_map.resolve(specifier, referrer)) + { + Some(Ok(value)) => return Ok(value), + Some(Err(err)) => Some(err), + None => None, + }; + // then with package.json if let Some(deps) = self.maybe_package_json_deps.as_ref() { if let Some(specifier) = resolve_package_json_dep(specifier, deps)? { return Ok(specifier); } - if let Some(req) = deps.get(specifier) { - return Ok(ModuleSpecifier::parse(&format!("npm:{req}")).unwrap()); - } } - deno_graph::resolve_import(specifier, referrer).map_err(|err| err.into()) + // otherwise, surface the import map error or try resolving when has no import map + if let Some(err) = maybe_import_map_err { + Err(err.into()) + } else { + deno_graph::resolve_import(specifier, referrer).map_err(|err| err.into()) + } } } From 4a81cf4279086b53f7597864f8f6005dddd87419 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Feb 2023 16:38:55 -0500 Subject: [PATCH 2/4] Tests --- cli/tests/integration/check_tests.rs | 10 ++++++++++ cli/tests/integration/run_tests.rs | 9 +++++++++ cli/tests/testdata/package_json/deno_json/deno.json | 5 +++++ .../testdata/package_json/deno_json/main.check.out | 11 +++++++++++ cli/tests/testdata/package_json/deno_json/main.out | 2 ++ cli/tests/testdata/package_json/deno_json/main.ts | 9 +++++++++ cli/tests/testdata/package_json/deno_json/other.ts | 3 +++ .../testdata/package_json/deno_json/package.json | 5 +++++ 8 files changed, 54 insertions(+) create mode 100644 cli/tests/testdata/package_json/deno_json/deno.json create mode 100644 cli/tests/testdata/package_json/deno_json/main.check.out create mode 100644 cli/tests/testdata/package_json/deno_json/main.out create mode 100644 cli/tests/testdata/package_json/deno_json/main.ts create mode 100644 cli/tests/testdata/package_json/deno_json/other.ts create mode 100644 cli/tests/testdata/package_json/deno_json/package.json diff --git a/cli/tests/integration/check_tests.rs b/cli/tests/integration/check_tests.rs index 021a536c42ab13..1273fbdce3dd34 100644 --- a/cli/tests/integration/check_tests.rs +++ b/cli/tests/integration/check_tests.rs @@ -251,3 +251,13 @@ itest!(package_json_fail_check { copy_temp_dir: Some("package_json/basic"), exit_code: 1, }); + +itest!(package_json_with_deno_json { + args: "check --quiet main.ts", + output: "package_json/deno_json/main.check.out", + cwd: Some("package_json/deno_json/"), + copy_temp_dir: Some("package_json/deno_json/"), + envs: env_vars_for_npm_tests_no_sync_download(), + http_server: true, + exit_code: 1, +}); diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index d89142c212798e..4b7869ef69542b 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -2793,6 +2793,15 @@ itest!(package_json_auto_discovered_for_npm_binary { http_server: true, }); +itest!(package_json_with_deno_json { + args: "run --quiet -A main.ts", + output: "package_json/deno_json/main.out", + cwd: Some("package_json/deno_json/"), + copy_temp_dir: Some("package_json/deno_json/"), + envs: env_vars_for_npm_tests_no_sync_download(), + http_server: true, +}); + itest!(wasm_streaming_panic_test { args: "run run/wasm_streaming_panic_test.js", output: "run/wasm_streaming_panic_test.js.out", diff --git a/cli/tests/testdata/package_json/deno_json/deno.json b/cli/tests/testdata/package_json/deno_json/deno.json new file mode 100644 index 00000000000000..8a89da280984b0 --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/deno.json @@ -0,0 +1,5 @@ +{ + "imports": { + "other": "./other.ts" + } +} diff --git a/cli/tests/testdata/package_json/deno_json/main.check.out b/cli/tests/testdata/package_json/deno_json/main.check.out new file mode 100644 index 00000000000000..53b6869c0388ca --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/main.check.out @@ -0,0 +1,11 @@ +error: TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. +const _strValue1: string = NUMBER_VALUE; + ~~~~~~~~~~ + at file:///[WILDCARD]/main.ts:8:7 + +TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. +const _strValue2: string = test.getValue(); + ~~~~~~~~~~ + at file:///[WILDCARD]/main.ts:9:7 + +Found 2 errors. diff --git a/cli/tests/testdata/package_json/deno_json/main.out b/cli/tests/testdata/package_json/deno_json/main.out new file mode 100644 index 00000000000000..1191247b6d9a20 --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/main.out @@ -0,0 +1,2 @@ +1 +2 diff --git a/cli/tests/testdata/package_json/deno_json/main.ts b/cli/tests/testdata/package_json/deno_json/main.ts new file mode 100644 index 00000000000000..7768ff3fc4bcd8 --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/main.ts @@ -0,0 +1,9 @@ +import { NUMBER_VALUE } from "other"; +import * as test from "@denotest/esm-basic"; + +test.setValue(2); +console.log(test.getValue()); + +// these should cause type errors +const _strValue1: string = NUMBER_VALUE; +const _strValue2: string = test.getValue(); diff --git a/cli/tests/testdata/package_json/deno_json/other.ts b/cli/tests/testdata/package_json/deno_json/other.ts new file mode 100644 index 00000000000000..997d84adfb2fc2 --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/other.ts @@ -0,0 +1,3 @@ +console.log(1); + +export const NUMBER_VALUE = 1; diff --git a/cli/tests/testdata/package_json/deno_json/package.json b/cli/tests/testdata/package_json/deno_json/package.json new file mode 100644 index 00000000000000..54ca824d645c5f --- /dev/null +++ b/cli/tests/testdata/package_json/deno_json/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@denotest/esm-basic": "*" + } +} From 2e53b424368767bc34deafe43ea44062445a6edc Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Feb 2023 16:45:52 -0500 Subject: [PATCH 3/4] Fix lsp when config changes --- cli/lsp/language_server.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index dfbb9c2e174c0a..680e2c91235cc7 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -811,7 +811,6 @@ impl Inner { fn update_config_file(&mut self) -> Result<(), AnyError> { self.maybe_config_file = None; - self.maybe_package_json = None; self.fmt_options = Default::default(); self.lint_options = Default::default(); @@ -1205,7 +1204,7 @@ impl Inner { .map(|f| self.url_map.normalize_url(&f.uri)) .collect(); - // if the current tsconfig has changed, we need to reload it + // if the current deno.json has changed, we need to reload it if let Some(config_file) = &self.maybe_config_file { if changes.contains(&config_file.specifier) { if let Err(err) = self.update_config_file() { @@ -1218,7 +1217,8 @@ impl Inner { } } if let Some(package_json) = &self.maybe_package_json { - if changes.contains(&package_json.specifier()) { + // always update the package json if the deno config changes + if touched || changes.contains(&package_json.specifier()) { if let Err(err) = self.update_package_json() { self.client.show_message(MessageType::WARNING, err).await; } @@ -1228,7 +1228,7 @@ impl Inner { // if the current import map, or config file has changed, we need to reload // reload the import map if let Some(import_map_uri) = &self.maybe_import_map_uri { - if changes.contains(import_map_uri) || touched { + if touched || changes.contains(import_map_uri) { if let Err(err) = self.update_import_map().await { self.client.show_message(MessageType::WARNING, err).await; } From 9bcaae0ce85562647401882b03f1f60713e61c15 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Feb 2023 16:46:48 -0500 Subject: [PATCH 4/4] reload reload -> reload --- cli/lsp/language_server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 680e2c91235cc7..ee91e045ba9c8e 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1225,7 +1225,7 @@ impl Inner { touched = true; } } - // if the current import map, or config file has changed, we need to reload + // if the current import map, or config file has changed, we need to // reload the import map if let Some(import_map_uri) = &self.maybe_import_map_uri { if touched || changes.contains(import_map_uri) {