feat(cli): allow bare specifier for builtin node module #20728

kt3k · 2023-09-28T15:43:27Z

This PR allows using bare specifiers for importing builtin node modules (ex. "fs" resolves to "node:fs"). When such resolution happens, the warnings like the below are shown.

Warning: Resolving "fs" as "node:fs" at file:///path/to/script.ts:1:16. If you want to use a built-in Node module, add a "node:" prefix.

This feature is implemented behind the unstable flag `--unstable-bare-node-builtins"

closes #20566

bartlomieju

Will the warning be printed for every occurence of import "fs" or only the first one?

dsherret · 2023-09-28T20:32:57Z

cli/resolver.rs

@@ -291,6 +295,12 @@ impl NpmResolver for CliGraphResolver {
    }
  }

+  fn on_resolve_bare_builtin_node_module(&self, module_name: &str) {
+    if !self.quiet {
+      eprintln!("Warning: Resolving \"{module_name}\" as \"node:{module_name}\". If you want to use a built-in Node module, add a \"node:\" prefix.")


We can get rid of the quiet property and just use the log::warn! macro. The --quiet flag will automatically work with log::warn!

cli/resolver.rs

cli/tests/integration/lsp_tests.rs

kt3k · 2023-09-30T20:17:13Z

~~Now I found a bug. When any import map is present, the new resolution doesn't seem working..~~ fixed in 91aaf73

…ed by import map

kt3k · 2023-10-02T05:28:23Z

cli/tests/testdata/run/092_import_map_unmapped_bare_specifier.ts.out

@@ -1,4 +1,4 @@
-error: Uncaught (in promise) TypeError: Relative import path "unmapped" not prefixed with / or ./ or ../ and not in import map from "file://[WILDCARD]/092_import_map_unmapped_bare_specifier.ts"


The last part of this error message was lost because of this commit (We now ignore ImportMapError::UnmappedBareSpecifier because that can't be handled by bare node specifier check implemented in deno_graph.

I drafted a PR to handle ImportMapError::UnmappedBareSpecifier in deno_graph, but I'm not sure it's worth the complexity

ry

@bartlomieju asked for a feature flag that would allow us to land this disabled in a patch release. I don't see that in this patch?

dsherret · 2023-10-02T14:11:28Z

@ry maybe we can just land it without a feature flag in a patch? denoland/deno_graph#295 (comment)

kt3k · 2023-10-04T03:28:24Z

Created an issue for feature flag system #20779

Let me check my understanding. Are we going to handle 3 inputs for checking whether the feature is enabled?

CLI flag --unstable-bare-node-specifier
Env var DENO_UNSTABLE=bare_node_specifier
features field of deno.json(c) "features": ["bare_node_specifier"]

dsherret · 2023-10-04T03:32:20Z

Yup, that sounds good to me.

bartlomieju · 2023-10-04T11:25:43Z

Created an issue for feature flag system #20779

Let me check my understanding. Are we going to handle 3 inputs for checking whether the feature is enabled?

CLI flag --unstable-bare-node-specifier

Env var DENO_UNSTABLE=bare_node_specifier

features field of deno.json(c) "features": ["bare_node_specifier"]

Just saw this comment. #20765 should unblock you for checking if the feature is enabled. I would suggest --unstable-bare-node-builtins.

kt3k · 2023-10-04T12:29:59Z

--unstable-bare-node-builtins sounds good to me

…d of config file

kt3k · 2023-10-05T11:49:56Z

This branch now reads the feature flag from 3 inputs: DENO_UNSTABLE_BARE_NODE_BUILTINS=1 env var, --unstable-bare-node-builtins flag, "unstable": ["bare-node-builtins"] field in deno.json. The LSP behavior can only be configured by unstable field of deno.json

kt3k · 2023-10-06T07:05:41Z

I'd like to avoid implementing --no-bare-node-builtins flag in this PR because the feature is only enabled when explicitly enabled by the user. I think we should implement that flag when the feature is automatically enabled by some condition.

bartlomieju · 2023-10-12T22:13:21Z

@kt3k you can now get an instance of FeatureChecker from CliFactory. Let me know if you need any help with this update.

dsherret · 2023-10-12T23:51:53Z

I'm curious why this needs to use the feature checker. I feel like what's done here is fine since this is not an unstable runtime feature? What would the code look like?

cli/args/flags.rs

cli/factory.rs

kt3k · 2023-10-13T08:13:15Z

I'm curious why this needs to use the feature checker.

Now feature_checker is moved to CliFactory in #20797, and it's ready when we create the resolver. So I think it makes sense to pass flag to feature_checker, and we can follow the same pattern for all unstable flags.

kt3k · 2023-10-13T08:16:56Z

Now CliOption#unstable_bare_node_builtins includes all necessary checks for the feature flag. The flag is passed to FeatureChecker at the initialization of it. CliGraphResolver uses FeatureChecker#check_feature for the availability of the feature.

dsherret · 2023-10-13T08:15:40Z

cli/args/mod.rs

@@ -1216,6 +1216,13 @@ impl CliOptions {
    self.flags.unstable
  }

+  pub fn unstable_bare_node_builtlins(&self) -> bool {
+    self.flags.unstable_bare_node_builtlins
+      || self.maybe_config_file().as_ref().map_or(false, |c| {


Personally I find map_or to be confusing to read because it reads backwards instead of left to right or top to bottom. I opened #18212 to remove them in the past.

Oh, we prefer map().unwrap_or(). Ok. I'll update.

dsherret · 2023-10-13T08:17:12Z

cli/factory.rs

@@ -563,6 +566,9 @@ impl CliFactory {
  pub fn feature_checker(&self) -> &Arc<FeatureChecker> {
    self.services.feature_checker.get_or_init(|| {
      let mut checker = FeatureChecker::default();
+      if self.options.unstable_bare_node_builtlins() {
+        checker.enable_feature("bare-node-builtins");
+      }


I'm not sure it makes sense to pass this to the feature checker unless the feature is being used with the runtime code. For example, in this case we could just call self.options.unstable_bare_node_builtins() above on line 380.

Maybe this is just an overhead. Updated

…init

kt3k · 2023-10-13T08:57:08Z

We currently support the following 3. Do these look fine? (Especially I'm unconfident about the casing of deno.json field syntax)

DENO_UNSTABLE_BARE_NODE_BUILTINS=1 (env)
--unstable-bare-node-builtins (cli arg)
"unstable": ["bare-node-builtins"] (deno.json field)

$ cat b.js
import path from "path"
console.log(path.resolve);
$ ./target/debug/deno run b.js                              
error: Relative import path "path" not prefixed with / or ./ or ../
If you want to use a built-in Node module, add a "node:" prefix (ex. "node:path").
    at file:///Users/kt3k/denoland/deno/b.js:1:18
$ ./target/debug/deno run --unstable-bare-node-builtins b.js
Warning: Resolving "path" as "node:path" at file:///Users/kt3k/denoland/deno/b.js:1:18. If you want to use a built-in Node module, add a "node:" prefix.
[Function: resolve]
$ DENO_UNSTABLE_NODE_BUILTINS=1 ./target/debug/deno run b.js 
Warning: Resolving "path" as "node:path" at file:///Users/kt3k/denoland/deno/b.js:1:18. If you want to use a built-in Node module, add a "node:" prefix.
[Function: resolve]
$ echo '{"unstable":["bare-node-builtins"]}' > deno.json
$ ./target/debug/deno run b.js                              
Warning: Resolving "path" as "node:path" at file:///Users/kt3k/denoland/deno/b.js:1:18. If you want to use a built-in Node module, add a "node:" prefix.
[Function: resolve]

dsherret · 2023-10-13T09:18:36Z

@kt3k looks good to me!

kt3k · 2023-10-17T23:23:01Z

@bartlomieju PTAL

bartlomieju

LGTM too!

feat(cli): allow bare specifier for builtin node module

db03ccf

kt3k requested review from ry and dsherret September 28, 2023 15:43

bartlomieju reviewed Sep 28, 2023

View reviewed changes

dsherret reviewed Sep 28, 2023

View reviewed changes

refactor: use log::warn!

fccf6ae

kt3k mentioned this pull request Sep 29, 2023

fix: pass range to on_resolve_bare_builtin_node_module denoland/deno_graph#297

Merged

kt3k added 2 commits September 30, 2023 21:00

show line and column numbers

04961d8

test: update lsp diagnostics expectation

6d37f6a

dsherret reviewed Sep 30, 2023

View reviewed changes

cli/tests/integration/lsp_tests.rs Outdated Show resolved Hide resolved

kt3k added 3 commits October 1, 2023 04:55

show warning on bare node specifier via lsp

100953b

remove dead code

78063b9

Merge branch 'main' into allow-bare-specifier-for-builtin-node-modules

a4308bb

kt3k added 4 commits October 1, 2023 05:36

allow bare node specifier when import map is present

91aaf73

do not show diagnostics when bare node specifiers are explicitly mapp…

27de632

…ed by import map

test: update import map error expectations

7c3a80a

Merge branch 'main' into allow-bare-specifier-for-builtin-node-modules

f706dec

kt3k force-pushed the allow-bare-specifier-for-builtin-node-modules branch from 889f830 to f706dec Compare October 2, 2023 03:46

kt3k mentioned this pull request Oct 2, 2023

fix: check bare node specifier for ImportMapError::UnmappedBareSpecifier denoland/deno_graph#298

Merged

kt3k commented Oct 2, 2023

View reviewed changes

ry reviewed Oct 2, 2023

View reviewed changes

kt3k added 2 commits October 2, 2023 23:23

use deno_graph 0.56.2, keep error from MappedSpecifierResolver

ee7261e

Merge branch 'main' into allow-bare-specifier-for-builtin-node-modules

9380156

kt3k mentioned this pull request Oct 4, 2023

fix: add is_bare_node_specifier_enabled to NpmResolver trait denoland/deno_graph#299

Merged

kt3k added 7 commits October 5, 2023 14:46

Merge branch 'main' into allow-bare-specifier-for-builtin-node-modules

91ff3ac

add feature flag

2cbd685

fix lsp test case

517ec83

restore --quiet option in test

cab2bba

enable feature by config

c8c5334

test lsp diagnostics with bare-node-builtins enabled by unstable fiel…

bb26d6c

…d of config file

fix lint

3b76e13

dsherret reviewed Oct 12, 2023

View reviewed changes

cli/args/flags.rs Outdated Show resolved Hide resolved

dsherret reviewed Oct 13, 2023

View reviewed changes

cli/factory.rs Outdated Show resolved Hide resolved

kt3k added 3 commits October 13, 2023 16:33

Merge branch 'main' into allow-bare-specifier-for-builtin-node-modules

8de32f5

use feature_checker, implement CliOption#unstable_bare_node_builtins

680b489

refactor: use map_or

accc12f

dsherret reviewed Oct 13, 2023

View reviewed changes

kt3k added 2 commits October 13, 2023 17:26

use self.options.unstable_bare_node_builtlins() directly in resolver …

54b6341

…init

use map().unwrap_or() instead of map_or()

9568be2

bartlomieju approved these changes Oct 19, 2023

View reviewed changes

kt3k merged commit fb73eb1 into denoland:main Oct 20, 2023
13 checks passed

kt3k deleted the allow-bare-specifier-for-builtin-node-modules branch October 20, 2023 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): allow bare specifier for builtin node module #20728

feat(cli): allow bare specifier for builtin node module #20728

kt3k commented Sep 28, 2023 •

edited

Loading

bartlomieju left a comment

dsherret Sep 28, 2023 •

edited

Loading

kt3k commented Sep 30, 2023 •

edited

Loading

kt3k Oct 2, 2023

ry left a comment

dsherret commented Oct 2, 2023

kt3k commented Oct 4, 2023

dsherret commented Oct 4, 2023

bartlomieju commented Oct 4, 2023

kt3k commented Oct 4, 2023

kt3k commented Oct 5, 2023

kt3k commented Oct 6, 2023

bartlomieju commented Oct 12, 2023

dsherret commented Oct 12, 2023 •

edited

Loading

kt3k commented Oct 13, 2023

kt3k commented Oct 13, 2023

dsherret Oct 13, 2023

kt3k Oct 13, 2023

dsherret Oct 13, 2023

kt3k Oct 13, 2023

kt3k commented Oct 13, 2023

dsherret commented Oct 13, 2023

kt3k commented Oct 17, 2023

bartlomieju left a comment

		@@ -1,4 +1,4 @@
		error: Uncaught (in promise) TypeError: Relative import path "unmapped" not prefixed with / or ./ or ../ and not in import map from "file://[WILDCARD]/092_import_map_unmapped_bare_specifier.ts"

feat(cli): allow bare specifier for builtin node module #20728

feat(cli): allow bare specifier for builtin node module #20728

Conversation

kt3k commented Sep 28, 2023 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

dsherret Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

kt3k commented Sep 30, 2023 • edited Loading

kt3k Oct 2, 2023

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

dsherret commented Oct 2, 2023

kt3k commented Oct 4, 2023

dsherret commented Oct 4, 2023

bartlomieju commented Oct 4, 2023

kt3k commented Oct 4, 2023

kt3k commented Oct 5, 2023

kt3k commented Oct 6, 2023

bartlomieju commented Oct 12, 2023

dsherret commented Oct 12, 2023 • edited Loading

kt3k commented Oct 13, 2023

kt3k commented Oct 13, 2023

dsherret Oct 13, 2023

Choose a reason for hiding this comment

kt3k Oct 13, 2023

Choose a reason for hiding this comment

dsherret Oct 13, 2023

Choose a reason for hiding this comment

kt3k Oct 13, 2023

Choose a reason for hiding this comment

kt3k commented Oct 13, 2023

dsherret commented Oct 13, 2023

kt3k commented Oct 17, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

kt3k commented Sep 28, 2023 •

edited

Loading

dsherret Sep 28, 2023 •

edited

Loading

kt3k commented Sep 30, 2023 •

edited

Loading

dsherret commented Oct 12, 2023 •

edited

Loading