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

fix(npm): allow resolving from package.json when an import map exists #17905

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions cli/lsp/language_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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() {
Expand All @@ -1218,17 +1217,18 @@ 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;
}
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 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;
}
Expand Down
26 changes: 17 additions & 9 deletions cli/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,22 +113,30 @@ impl Resolver for CliGraphResolver {
specifier: &str,
referrer: &ModuleSpecifier,
) -> Result<ModuleSpecifier, AnyError> {
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());
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I accidentally left this in last time.

}

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())
}
}
}

Expand Down
10 changes: 10 additions & 0 deletions cli/tests/integration/check_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
9 changes: 9 additions & 0 deletions cli/tests/integration/run_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
5 changes: 5 additions & 0 deletions cli/tests/testdata/package_json/deno_json/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"imports": {
"other": "./other.ts"
}
}
11 changes: 11 additions & 0 deletions cli/tests/testdata/package_json/deno_json/main.check.out
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 2 additions & 0 deletions cli/tests/testdata/package_json/deno_json/main.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
1
2
9 changes: 9 additions & 0 deletions cli/tests/testdata/package_json/deno_json/main.ts
Original file line number Diff line number Diff line change
@@ -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();
3 changes: 3 additions & 0 deletions cli/tests/testdata/package_json/deno_json/other.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
console.log(1);

export const NUMBER_VALUE = 1;
5 changes: 5 additions & 0 deletions cli/tests/testdata/package_json/deno_json/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"@denotest/esm-basic": "*"
}
}