feat: resolve node builtin module without node: scheme

[kt3k]

part of denoland/deno#20566

This PR updates the resolution of bare specifiers. If the module specifier is node builtin module as a bare specifier, resolve it as node:<module_name>. Also it prints the warning message like Warning: Resolving "path" as "node:path". If you want to use a built-in Node module, add a "node:" prefix.

[kt3k]

Tested with the below change in Deno CLI repo:

index 36dccab10..6bbdc96c0 100644
--- a/cli/Cargo.toml
+++ b/cli/Cargo.toml
@@ -49,16 +49,16 @@ deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_gra
 deno_cache_dir = "=0.6.0"
 deno_config = "=0.3.1"
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
-deno_doc = "=0.67.0"
-deno_emit = "=0.28.0"
-deno_graph = "=0.55.0"
+deno_doc = { path = "../../deno_doc" }
+deno_emit = { path = "../../deno_emit/rs-lib" }
+deno_graph = { path = "../../deno_graph" }
 deno_lint = { version = "=0.51.0", features = ["docs"] }
 deno_lockfile.workspace = true
 deno_npm.workspace = true
 deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "exclude_runtime_main_js", "include_js_files_for_snapshotting"] }
 deno_semver.workspace = true
 deno_task_shell = "=0.13.2"
-eszip = "=0.53.0"
+eszip = { path = "../../eszip" }
 napi_sym.workspace = true
 
 async-trait.workspace = true

It works like the below:

$ cat a.js                       
import fs from "fs";
import path from "path";
console.log(fs.write);
$ ./target/debug/deno run a.js 
Warning: Resolving "fs" as "node:fs". If you want to use a built-in Node module, add a "node:" prefix.
Warning: Resolving "path" as "node:path". If you want to use a built-in Node module, add a "node:" prefix.
[Function: write]

[dsherret]

@kt3k looks mostly good, but we need to make some changes.

[kt3k]

Now this change uses is_builtin_node_module_name and on_resolve_bare_builtin_node_module from NpmResolver trait for checking builtin node module and printing warnings.

I skipped supporting the same thing with wasm/js version as npm resolver doesn't seem configured for them.

[dsherret]

Very close now. Looking good!

[dsherret]

I think we should move this check to a higher level in:

deno_graph/src/graph.rs

Lines 1551 to 1562 in aa9236c

	fn resolve(
	specifier_text: &str,
	referrer_range: Range,
	maybe_resolver: Option<&dyn Resolver>,
	) -> Resolution {
	let response = if let Some(resolver) = maybe_resolver {
	resolver.resolve(specifier_text, &referrer_range.specifier)
	} else {
	resolve_import(specifier_text, &referrer_range.specifier)
	.map_err(|err| err.into())
	};
	Resolution::from_resolve_result(response, specifier_text, referrer_range)

#296 will make this easier.

That way this behaviour isn't dependent on an implentor of the Resolver calling this resolve_import function and needing access to the npm resolver. Basically, if a user provides an NpmResolver to deno_graph then they'll get this behaviour.

[kt3k]

Moved the check of bare builtin node specifier. Now it is checked after resolver.resolve is applied.

[dsherret]

I know we discussed making this conditional in the meeting, but maybe making it "just work" is ok to do in a patch release of Deno. It would be less work and make stuff work sooner. cc @bartlomieju

[bartlomieju]

That's okay for me 👍

[dsherret]

Nice! LGTM!

[dsherret]

FWIW, the .unwrap() could be removed by using .filter on the result of ModuleSpecifier::parse(&format!("node:{}", specifier_text)).ok() when npm_resolver.resolve_builtin_node_module(s).ok().flatten().is_some()

[kt3k]

FWIW, the .unwrap() could be removed by using .filter on the result of ModuleSpecifier::parse(&format!("node:{}", specifier_text)).ok()

I didn't get what this exactly means.., but removed the usage of .unwrap() in a different way. 4ebda53

[kt3k]

@dsherret Thank you for your review! I'll land this and release it as v0.56.0 tomorrow my time (and will continue working on dependent crates, such as eszip, deno_emit, deno_doc, etc).

[kt3k]

denoland/eszip#149
denoland/deno_doc#352
denoland/deno_emit#140