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

bartlomieju · 2021-10-01T15:42:55Z

~~Currently blocked on denoland/import_map#11~~

cli/node_compat.rs

ry

LGTM - quick and dirty, that's how I like MVPs.

kitsonk · 2021-10-02T19:19:24Z

cli/proc_state.rs

+        }
+      };
+      let node_builtins = crate::compat::get_mapped_node_builtins();
+      let diagnostics = import_map.update_imports(node_builtins)?;


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

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"

Added a test: compat::existing_import_map

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

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?

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.

bartlomieju · 2021-10-02T20:06:00Z

Before landing I think we should add aliases for node: prefix as per: https://nodejs.org/api/esm.html#esm_node_imports

kitsonk

I am all for moving fast, but a few concerns about this PR:

As it stands, this is something that takes something that is either not used by the user or explicitly used by a user and modifies it in an opaque way. Not only is this bad for the user (I expected x behaviour but suddenly it does y), but there is no way to fix it.
As it stands, this doesn't do anything that the user can't do a runtime. We have been endlessly debating that the CLI should do less and that we shouldn't introduce things (or even remove things) that make it inconsistent with Deploy. Deploy could easily support impart maps, which if generated could easily be used without introducing a --compat flag for Deploy. So why does this need to be a flag in the CLI?
This creates more problems like --unstable in that authors of code would need to tell consumers to run it. It all being in an import map doesn't solve that problem specifically, but we need to address "import map composition" anyways, and it is far easier to address if there isn't some code behind the scenes modifying it in an opaque way.
There is no support in the lsp for this. Meaning someone using this flag has no way to set it in the editor, meaning that the behaviour is quite a bit different.

We should really consider this as a standalone, runtime tool first, getting experience with the problems and issues encountered before we bake it into the CLI, IMO.

It would be great if we could talk about these things in our design meeting, before we commit to delivering them.

kitsonk · 2021-10-02T20:42:39Z

cli/compat.rs

+
+  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);


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?

Yep - we can worry about this later.

kitsonk · 2021-10-02T20:43:39Z

We should really consider this as a standalone, runtime tool first, getting experience with the problems and issues encountered before we bake it into the CLI, IMO.

Which if it were, you would get lsp support immediately.

ry · 2021-10-02T23:28:37Z

So why does this need to be a flag in the CLI?

@kitsonk to make it fast and easy.

So we've got a couple todos:

"node:" prefixes. I think this could as well be done in follow up PR. Most code does not use these prefixes.
Figure out X-Deno-Warning and figure out if we should tag std URLs. Could be done in follow up.
LSP support. Could be done in follow up.

piscisaureus

I have reservations about the use of a flag, long term.

However I do believe this is the right approach for making more of the existing node.js ecosystem available to Deno users.

wojpawlik · 2021-10-05T11:40:01Z

What's the advantage of --compat over hypothetical --import-map=https://deno.land/std/node/import_map.json?

Before landing I think we should add aliases for node: prefix as per: nodejs.org/api/esm.html#esm_node_imports

I agree! Those could even be available without any flags.

So why does this need to be a flag in the CLI?

to make it fast and easy.

I believe this is a mistake. Deno used to be very deliberate when adding features.

bartlomieju · 2021-10-05T12:44:04Z

What's the advantage of --compat over hypothetical --import-map=https://deno.land/std/node/import_map.json?

--compat adds relevant entries to the import map on the fly, having https://deno.land/std/node/import_map.json means you need to manually construct import map.

I agree! Those could even be available without any flags.

Why would they be available without a flag? We support only http:, https: and file: schemes by default, given that node: is not really popular in Node.js ecosystem I see no point in supporting that without a flag.

I believe this is a mistake. Deno used to be very deliberate when adding features.

Thanks for the feedback. This is an experiment, it might never ship.

wojpawlik · 2021-10-05T16:43:36Z

having https://deno.land/std/node/import_map.json means you need to manually construct import map.

Not if you allow to reference one import map within another, or repeat --import-map.

Why would they be available without a flag?

Because they'd create a subset of ESM that could be run in both runtimes with no changes or flags, without conflicting with anything.

node: is not really popular

Node.js uses it in docs now, so I'd expect that to start changing once Node 14 hits EOL. And Deno support could boost adoption.

bartlomieju · 2021-10-05T17:00:13Z

having https://deno.land/std/node/import_map.json means you need to manually construct import map.

Not if you allow to reference one import map within another, or repeat --import-map.

Import map spec doesn't allow that right now, if it changes this is definitely something to consider.

Why would they be available without a flag?

Because they'd create a subset of ESM that could be run in both runtimes with no changes or flags, without conflicting with anything.

Sure, but I haven't seen any libraries using node: prefix right now, if that changes and becomes popular we can consider that.

node: is not really popular

Node.js uses it in docs now, so I'd expect that to start changing once Node 14 hits EOL. And Deno support could boost adoption.

Sure, so we'll support that via the --compat flag right now.

wojpawlik · 2021-10-05T17:03:22Z

Sure, so we'll support that via the --compat flag right now.

Fair enough. Please mark the flag as unstable tho, so it can be removed without notice as soon as it's no longer needed.

cmoog · 2021-10-12T21:28:28Z

quick and dirty, that's how I like MVPs.

This sets a bad precedent for the Deno CLI as a testing ground for experiments. Can you imagine the Go team rushing quick and dirty features into the Go CLI?

We should really consider this as a standalone, runtime tool first, getting experience with the problems and issues encountered before we bake it into the CLI, IMO.

^^ agreed.

dsherret · 2021-10-12T21:35:55Z

This sets a bad precedent for the Deno CLI as a testing ground for experiments. Can you imagine the Go team rushing quick and dirty features into the Go CLI?

@cmoog please note that this is an unstable feature. Additionally, it has been discussed endlessly—it was not rushed and it will be developed/improved more over time.

bartlomieju added 2 commits October 1, 2021 17:42

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

4f530c4

get some mappings working

ac5bc75

dsherret reviewed Oct 1, 2021

View reviewed changes

cli/node_compat.rs Outdated Show resolved Hide resolved

ry mentioned this pull request Oct 2, 2021

basic node compatibity mode #12295

Closed

7 tasks

--node-compat -> --compat

2ff2fd5

bartlomieju changed the title ~~[WIP] feat: add --node-compat flag to provide built-in Node modules~~ [WIP] feat: add --compat flag to provide built-in Node modules Oct 2, 2021

bartlomieju changed the title ~~[WIP] feat: add --compat flag to provide built-in Node modules~~ feat: add --compat flag to provide built-in Node modules Oct 2, 2021

lint

60e6949

bartlomieju requested a review from ry October 2, 2021 14:07

ry and others added 7 commits October 2, 2021 12:56

s/node_compat/compat

2e55a29

add support for promises modules

d1ccdf3

print out diagnotics from updated import map

5e2c2ad

don't store url in static

5c7a075

fix

947fc4e

add integration test

154f949

Add more supported modules

24923bc

ry approved these changes Oct 2, 2021

View reviewed changes

kitsonk reviewed Oct 2, 2021

View reviewed changes

bartlomieju mentioned this pull request Oct 2, 2021

feat: add missing node modules denoland/std#1337

Merged

add test for existing import map

71ca413

kitsonk reviewed Oct 2, 2021

View reviewed changes

clarify import map diagnostics

17668f9

bartlomieju mentioned this pull request Oct 4, 2021

Add documentation about --compat flag denoland/manual#105

Closed

piscisaureus approved these changes Oct 4, 2021

View reviewed changes

bartlomieju merged commit f1d3a17 into denoland:main Oct 4, 2021

bartlomieju deleted the node_builtins branch October 4, 2021 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bartlomieju commented Oct 1, 2021 •

edited

Loading

ry left a comment

kitsonk Oct 2, 2021

bartlomieju Oct 2, 2021

bartlomieju Oct 2, 2021

bartlomieju Oct 2, 2021

kitsonk Oct 2, 2021

bartlomieju Oct 3, 2021

kitsonk Oct 3, 2021

bartlomieju commented Oct 2, 2021

kitsonk left a comment

kitsonk Oct 2, 2021

ry Oct 2, 2021

kitsonk commented Oct 2, 2021

ry commented Oct 2, 2021

piscisaureus left a comment

wojpawlik commented Oct 5, 2021

bartlomieju commented Oct 5, 2021

wojpawlik commented Oct 5, 2021

bartlomieju commented Oct 5, 2021

wojpawlik commented Oct 5, 2021

cmoog commented Oct 12, 2021

dsherret commented Oct 12, 2021

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

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

Conversation

bartlomieju commented Oct 1, 2021 • edited Loading

ry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Oct 2, 2021

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk commented Oct 2, 2021

ry commented Oct 2, 2021

piscisaureus left a comment

Choose a reason for hiding this comment

wojpawlik commented Oct 5, 2021

bartlomieju commented Oct 5, 2021

wojpawlik commented Oct 5, 2021

bartlomieju commented Oct 5, 2021

wojpawlik commented Oct 5, 2021

cmoog commented Oct 12, 2021

dsherret commented Oct 12, 2021

bartlomieju commented Oct 1, 2021 •

edited

Loading