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

feat: add --compat flag to provide built-in Node modules #12293

Merged
merged 13 commits into from
Oct 4, 2021
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ encoding_rs = "0.8.28"
env_logger = "0.8.4"
fancy-regex = "0.7.1"
http = "0.2.4"
import_map = "0.3.0"
import_map = "0.3.3"
jsonc-parser = { version = "0.17.0", features = ["serde"] }
lazy_static = "1.4.0"
libc = "0.2.101"
Expand Down
60 changes: 60 additions & 0 deletions cli/compat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

use std::collections::HashMap;

static SUPPORTED_MODULES: &[&str] = &[
"assert",
"assert/strict",
"async_hooks",
"buffer",
"child_process",
"cluster",
"console",
"constants",
"crypto",
"dgram",
"dns",
"domain",
"events",
"fs",
"fs/promises",
"http",
"https",
"module",
"net",
"os",
"path",
"path/posix",
"path/win32",
"perf_hooks",
"process",
"querystring",
"readline",
"stream",
"stream/promises",
"stream/web",
"string_decoder",
"sys",
"timers",
"timers/promises",
"tls",
"tty",
"url",
"util",
"util/types",
"v8",
"vm",
"zlib",
];

pub fn get_mapped_node_builtins() -> HashMap<String, String> {
let mut mappings = HashMap::new();

for module in SUPPORTED_MODULES {
// TODO(bartlomieju): this is unversioned, and should be fixed to use latest stable?
let module_url = format!("https://deno.land/std/node/{}.ts", module);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not going to print the X-Deno-Warning text every time the user caches it for the first time, with no clue as how to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

Yep - we can worry about this later.

mappings.insert(module.to_string(), module_url);
}

mappings
}
32 changes: 32 additions & 0 deletions cli/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ pub struct Flags {
pub log_level: Option<Level>,
pub no_check: bool,
pub no_remote: bool,
/// If true, a list of Node built-in modules will be injected into
/// the import map.
pub compat: bool,
pub prompt: bool,
pub reload: bool,
pub repl: bool,
Expand Down Expand Up @@ -1490,6 +1493,7 @@ fn runtime_args<'a, 'b>(
.arg(v8_flags_arg())
.arg(seed_arg())
.arg(enable_testing_features_arg())
.arg(compat_arg())
}

fn inspect_args<'a, 'b>(app: App<'a, 'b>) -> App<'a, 'b> {
Expand Down Expand Up @@ -1619,6 +1623,12 @@ fn seed_arg<'a, 'b>() -> Arg<'a, 'b> {
})
}

fn compat_arg<'a, 'b>() -> Arg<'a, 'b> {
Arg::with_name("compat")
.long("compat")
.help("Node compatibility mode. Currently only enables built-in node modules like 'fs'.")
}

fn watch_arg<'a, 'b>() -> Arg<'a, 'b> {
Arg::with_name("watch")
.long("watch")
Expand Down Expand Up @@ -2228,6 +2238,7 @@ fn runtime_args_parse(
location_arg_parse(flags, matches);
v8_flags_arg_parse(flags, matches);
seed_arg_parse(flags, matches);
compat_arg_parse(flags, matches);
inspect_arg_parse(flags, matches);
enable_testing_features_arg_parse(flags, matches);
}
Expand Down Expand Up @@ -2313,6 +2324,12 @@ fn seed_arg_parse(flags: &mut Flags, matches: &ArgMatches) {
}
}

fn compat_arg_parse(flags: &mut Flags, matches: &ArgMatches) {
if matches.is_present("compat") {
flags.compat = true;
}
}

fn no_check_arg_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
if matches.is_present("no-check") {
flags.no_check = true;
Expand Down Expand Up @@ -4431,4 +4448,19 @@ mod tests {
.to_string()
.contains("Expected protocol \"http\" or \"https\""));
}

#[test]
fn compat() {
let r = flags_from_vec(svec!["deno", "run", "--compat", "foo.js"]);
assert_eq!(
r.unwrap(),
Flags {
subcommand: DenoSubcommand::Run(RunFlags {
script: "foo.js".to_string(),
}),
compat: true,
..Flags::default()
}
);
}
}
1 change: 1 addition & 0 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
mod ast;
mod auth_tokens;
mod checksum;
mod compat;
mod config_file;
mod deno_dir;
mod diagnostics;
Expand Down
29 changes: 28 additions & 1 deletion cli/proc_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use deno_tls::rustls_native_certs::load_native_certs;
use deno_tls::webpki_roots::TLS_SERVER_ROOTS;
use import_map::ImportMap;
use log::debug;
use log::info;
use log::warn;
use std::collections::HashMap;
use std::collections::HashSet;
Expand Down Expand Up @@ -182,7 +183,7 @@ impl ProcState {
None
};

let maybe_import_map: Option<ImportMap> =
let mut maybe_import_map: Option<ImportMap> =
match flags.import_map_path.as_ref() {
None => None,
Some(import_map_url) => {
Expand All @@ -204,6 +205,32 @@ impl ProcState {
}
};

if flags.compat {
let mut import_map = match maybe_import_map {
Some(import_map) => import_map,
None => {
// INFO: we're creating an empty import map, with its specifier pointing
// to `CWD/node_import_map.json` to make sure the map still works as expected.
let import_map_specifier =
std::env::current_dir()?.join("node_import_map.json");
ImportMap::from_json(import_map_specifier.to_str().unwrap(), "{}")
.unwrap()
}
};
let node_builtins = crate::compat::get_mapped_node_builtins();
let diagnostics = import_map.update_imports(node_builtins)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if some already has entries for some/all of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

A diagnostic is printed out to the terminal, like so:

❯ cargo run -- run --compat --unstable -A --import-map import-map.json cli/tests/testdata/compat/fs_promises.js
    Finished dev [unoptimized + debuginfo] target(s) in 0.38s
     Running `target/debug/deno run --compat --unstable -A --import-map import-map.json cli/tests/testdata/compat/fs_promises.js`
  - "fs" already exists and is mapped to Some(Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("example.com")), port: None, path: "/1", query: None, fragment: None })

I just noticed that the output is broken and will fix it before landing the PR, should be

  - "fs" already exists and is mapped to "https://example.com/1"

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test: compat::existing_import_map

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally shows up as:

The were problems adding Node built-ins to import map:
  - "fs/promises" already exists and is mapped to "[WILDCARD]non_existent_file.js"

Suggestions welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is still unclear to the user what the state is or what action they should take. Is it really a problem? If it is a problem, what should a user do about it to resolve it?

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 clarified it even further:

Some Node built-ins were not added to the import map:
  - "fs/promises" already exists and is mapped to "[WILDCARD]non_existent_file.js"
If you want to use Node built-ins provided by Deno remove listed specifiers from "imports" mapping in the import map file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool


if !diagnostics.is_empty() {
info!("Some Node built-ins were not added to the import map:");
for diagnostic in diagnostics {
info!(" - {}", diagnostic);
}
info!("If you want to use Node built-ins provided by Deno remove listed specifiers from \"imports\" mapping in the import map file.");
}

maybe_import_map = Some(import_map);
}

let maybe_inspect_host = flags.inspect.or(flags.inspect_brk);
let maybe_inspector_server = maybe_inspect_host.map(|host| {
Arc::new(InspectorServer::new(host, version::get_user_agent()))
Expand Down
14 changes: 14 additions & 0 deletions cli/tests/integration/compat_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

use crate::itest;

itest!(fs_promises {
args: "run --compat --unstable -A compat/fs_promises.js",
output: "compat/fs_promises.out",
});

itest!(existing_import_map {
args: "run --compat --import-map compat/existing_import_map.json compat/fs_promises.js",
output: "compat/existing_import_map.out",
exit_code: 1,
});
2 changes: 2 additions & 0 deletions cli/tests/integration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ macro_rules! itest_flaky(
mod bundle;
#[path = "cache_tests.rs"]
mod cache;
#[path = "compat_tests.rs"]
mod compat;
#[path = "compile_tests.rs"]
mod compile;
#[path = "coverage_tests.rs"]
Expand Down
5 changes: 5 additions & 0 deletions cli/tests/testdata/compat/existing_import_map.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"imports": {
"fs/promises": "./non_existent_file.js"
}
}
5 changes: 5 additions & 0 deletions cli/tests/testdata/compat/existing_import_map.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[WILDCARD]
Some Node built-ins were not added to the import map:
- "fs/promises" already exists and is mapped to "[WILDCARD]non_existent_file.js"
If you want to use Node built-ins provided by Deno remove listed specifiers from "imports" mapping in the import map file.
error: Cannot resolve module [WILDCARD]
3 changes: 3 additions & 0 deletions cli/tests/testdata/compat/fs_promises.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import fs from "fs/promises";
const data = await fs.readFile("compat/test.txt", "utf-8");
console.log(data);
2 changes: 2 additions & 0 deletions cli/tests/testdata/compat/fs_promises.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[WILDCARD]
This is some example text that will be read using compatiblity mode.
1 change: 1 addition & 0 deletions cli/tests/testdata/compat/test.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This is some example text that will be read using compatiblity mode.
1 change: 1 addition & 0 deletions cli/tools/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ pub fn compile_to_runtime_flags(
lock: None,
log_level: flags.log_level,
no_check: false,
compat: flags.compat,
unsafely_ignore_certificate_errors: flags
.unsafely_ignore_certificate_errors,
no_remote: false,
Expand Down