feat(compat): Node CJS and ESM resolvers by bartlomieju · Pull Request #12424 · denoland/deno

bartlomieju · 2021-10-13T05:51:29Z

So now all methods are ported, there are a few missing bits:

checking for loader shouldn't reqiuire --allow-net permission
cache read package.json (~~need to figure out if Node does that globally or per-isolate~~ it's per isolate)
2 functions that emit deprecation warnings
add moooooore tests
figure out if the integration is fine, there is a single Deno specific conditional that short-circuits in "finalize_resolution" to handle HTTP(S) imports
setup compat mode in different CLI subcommands
setup compat mode in worker code (can wait until feat(node): worker_threads std#1151 lands)

bartlomieju · 2021-10-13T16:42:50Z

So I removed injection of import map when --compat is passed, instead directly resolving built-in Node modules in the NodeEsmResolver. However I'm having a slight problem with type-checking:

Check file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts
error: TS2304 [ERROR]: Cannot find name 'global'.
if (global != window) {
    ~~~~~~
    at file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts:1:5

TS2580 [ERROR]: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.
console.log(process);
            ~~~~~~~
    at file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts:5:13

TS2580 [ERROR]: Cannot find name 'Buffer'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.
console.log(Buffer);
            ~~~~~~
    at file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts:6:13

TS2304 [ERROR]: Cannot find name 'setImmediate'.
console.log(setImmediate);
            ~~~~~~~~~~~~
    at file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts:7:13

TS2304 [ERROR]: Cannot find name 'clearImmediate'.
console.log(clearImmediate);
            ~~~~~~~~~~~~~~
    at file:///Users/biwanczuk/dev/deno/cli/tests/testdata/compat/globals.ts:8:13

Found 5 errors.

Ie. globals are no longer part of type-checked module graph, even though they are still injected using get_maybe_imports(). @kitsonk could you take a look?

bartlomieju · 2021-10-13T16:47:52Z

+    // TODO(bartlomieju): this is hacky, remove
+    // needed to add it here because `deno_std/node` has
+    // triple-slash references and they should still resolve
+    // the regular way (I think)
+    if referrer.as_str().starts_with("https://deno.land/std") {
+      return referrer.join(specifier).map_err(AnyError::from);
+    }


I think the type checking problem might be somehow related to this bit...

Actually, I think the problem stems from the fact that deno.land/std/node/global.ts is not part of the graph roots...

It will be part of the graph, but I think the problem is that it isn't being "imported" anywhere from the view of TypeScript. So while the module is available to be loaded, it doesn't get loaded, and therefore its globals don't get added to the global scope.

So there are couple options, I think:

Use a synthetic module like we do for eval, to do the importing.

We create a node/globals.d.ts that is an ambient declaration and we import that into the graph.

While I am not a fan of the synthetic modules, I think it might be the only way to do it. It would also mean you don't have to do any "side loading" of modules either, as the synthetic module should import everything as the "root" of the compat graph.

Thanks! The synthetic module approach seems reasonable to me. I will look up some existing examples and try to fix it.

So I just manually added node/global.ts to roots that is passed to the module graph. This is probably ugly solution but works for now. I was unable to figure out how to do "synthetic module".

bartlomieju · 2021-10-17T23:40:44Z

I want to land this PR before 1.15.2 is released (so probably tomorrow), let me know if you want me to integrate this resolution in all possible subcommands or is it enough just for deno run <file> for the first pass.

The CI will stay red until new version of deno_std is released as one test checks against STD url.

@kitsonk @dsherret I would appreciate reviews, especially around integration with deno_graph - I think this is still wrong and sometimes I see multiple Check <deno_std>/global.ts messages.

I believe we can punt on the remaining points from the first comment till next release.

kitsonk · 2021-10-18T04:35:21Z

+  Ok(url)
+}
+
+fn to_file_path(url: &Url) -> PathBuf {


I believe we have these sorts of things elsewhere in the code base. Is there are specific reason why we aren't DRY-ing this up?

I ported everything from codebase as-is to have the least amount of divergence; I guess we could DRY this up in a follow up?

kitsonk · 2021-10-18T04:42:29Z

    let maybe_locker = as_maybe_locker(self.lockfile.clone());
    let maybe_imports = self.get_maybe_imports();
-    let maybe_resolver =
+    let node_resolver = NodeEsmResolver;


So you can't use import maps with --compat mode?

Ideally the NodeEsmResolver would take an optional Resolver that it backs off to.

So you can't use import maps with --compat mode?

Currently no, I haven't seen a Node package that used import map. Is there a concrete use case right now to support import map?

Ideally the NodeEsmResolver would take an optional Resolver that it backs off to.

I'm not sure what you mean, could you expand on that? Are you suggesting that NodeEsmResolver should fallback to our regular module resolution? (And in turn support http(s) imports too)

Is there a concrete use case right now to support import map?

What happens when someone specifies --compat --importmap import_map.json?

I'm not sure what you mean, could you expand on that? Are you suggesting that NodeEsmResolver should fallback to our regular module resolution? (And in turn support http(s) imports too)

I am not exactly sure what I am saying. So you are saying the objective of --compat mode is to not allow existing Deno code to run? So there is no way anyone can use --compat as a temporary crutch and that they have to have all their dependencies loaded in node_modules with no opportunity to use the Deno features like remote imports, data URLs, blob URLs, import maps, etc?

Is there a concrete use case right now to support import map?

What happens when someone specifies --compat --importmap import_map.json?

Right now nothing, import map will be ignored - I can make the two flags mutually exclusive for now.

I'm not sure what you mean, could you expand on that? Are you suggesting that NodeEsmResolver should fallback to our regular module resolution? (And in turn support http(s) imports too)

I am not exactly sure what I am saying. So you are saying the objective of --compat mode is to not allow existing Deno code to run? So there is no way anyone can use --compat as a temporary crutch and that they have to have all their dependencies loaded in node_modules with no opportunity to use the Deno features like remote imports, data URLs, blob URLs, import maps, etc?

At the moment this is the case - --compat doesn't try to marry the two resolutions. That said data URLs and blob URLs should still work, because they are supported by Node. In theory this is a simple change to make - we could try to do regular resolution before falling back to Node resolution, however I'm unsure right now of ramifications of such decision. I'll be happy to discuss this in detail on the next meeting, but I'm feeling like it's something we could address in the next iteration.

kitsonk · 2021-10-18T04:46:15Z

+      import_map_resolver.as_ref().map(|im| im.as_resolver())
+    };
+    // TODO(bartlomieju): this is very make-shift, is there an existing API
+    // that we could include it like with "maybe_imports"?


I do think there is a way, I think we need to modify the logic that gets the roots for tsc.

I would create an issue and assign it to me to clean up post this PR.

ry · 2021-10-18T14:31:09Z

+// each release, a better mechanism is preferable, but it's a quick and dirty
+// solution to avoid printing `X-Deno-Warning` headers when the compat layer is
+// downloaded
+static STD_URL_STR: &str = "https://raw.githubusercontent.com/denoland/deno_std/b83eb65244c676608a69205569bd294113476464/";


Why use raw.githubusercontent instead of deno.land?

Because the code used is not yet available in an std release. I assume Bartek will update this during the release process once std has been cut.

denoland/std#1427

This circular dependency is obviously not ideal. I think we'll have to bring std/node/module.ts into CLI... but let's release like this for now.

This circular dependency is obviously not ideal. I think we'll have to bring std/node/module.ts into CLI... but let's release like this for now.

std/node/module.ts depends on other modules. I think we should just bring all of std/node into the CLI.

ry

LGTM

bartlomieju added 12 commits October 10, 2021 14:09

temp

29883b3

parse_package_name

9200bb2

module_resolve

becf749

package_exports_resolve

96a84d3

resolve_package_target_string

371ffcc

Merge branch 'main' into compat_node_resolver

0f62c43

node_resolve

dfd1190

chalk example working

6954cf6

legacy_main_resolve

a0b1ba6

pass conditions

5d41393

rename, remove debug prints

03ef6f7

clippy

ccf73ec

bartlomieju mentioned this pull request Oct 13, 2021

basic node compatibity mode #12295

Closed

7 tasks

ry and others added 4 commits October 13, 2021 08:03

add tests

34bb541

add deep test

bea885a

fmt

bba059a

remove import map

89e11b3

bartlomieju commented Oct 13, 2021

View reviewed changes

bartlomieju added 2 commits October 13, 2021 19:31

port errors

349804c

lint

be08d09

bartlomieju commented Oct 13, 2021

View reviewed changes

Comment thread cli/compat/mod.rs Outdated

Comment thread cli/compat/mod.rs Outdated

Comment thread cli/compat/mod.rs Outdated

bartlomieju added 8 commits October 13, 2021 21:11

revert pub modifiers, rename module

fe5460f

fix tests

ef6f6d3

add helpers

28efb3e

finish resolve_package_target_string

13079df

err_package_not_exported

57f3323

add missing methods

8e0a80b

add error code

efdfd6a

reset CI

3696f95

bartlomieju mentioned this pull request Oct 16, 2021

feat(node): CJS-ESM integration denoland/std#1412

Merged

bartlomieju added 5 commits October 16, 2021 17:04

add deno condition

f2104fa

use raw.githubusercontent.com

c1c49d0

Merge branch 'main' into compat_node_resolver

6d9b808

don't require --allow-net permission for compat mode

4c855f2

add test for conditional exports

43984e4

bartlomieju requested review from dsherret, kitsonk and ry October 17, 2021 23:38

update reference to deno_std

cfb47bd

kitsonk reviewed Oct 18, 2021

View reviewed changes

Merge branch 'main' into compat_node_resolver

b583bd7

ry reviewed Oct 18, 2021

View reviewed changes

ry added 2 commits October 18, 2021 10:31

small clean ups

62dd1c6

add test_is_relative_specifier

b32b4cd

dsherret reviewed Oct 18, 2021

View reviewed changes

Comment thread cli/compat/esm_resolver.rs Outdated

Comment thread cli/compat/esm_resolver.rs Outdated

Comment thread cli/compat/esm_resolver.rs Outdated

Comment thread cli/compat/esm_resolver.rs

Comment thread cli/compat/esm_resolver.rs Outdated

review comments

09e9dae

bartlomieju changed the title ~~[WIP] feat(compat): Node CJS and ESM resolvers~~ feat(compat): Node CJS and ESM resolvers Oct 18, 2021

bartlomieju requested a review from ry October 18, 2021 15:49

fix test

8c39946

ry approved these changes Oct 18, 2021

View reviewed changes

bartlomieju and others added 5 commits October 18, 2021 18:05

Merge branch 'main' into compat_node_resolver

a3f6a61

fix test

f357f98

Escape injected string.

fa59547

Escape other injected single quote string too.

cb1652c

Remove accidental println

f6686da

bartlomieju merged commit 617eeab into denoland:main Oct 18, 2021

bartlomieju deleted the compat_node_resolver branch October 18, 2021 17:36

ealib mentioned this pull request Oct 19, 2021

Node compatibility #2644

Closed

Conversation

bartlomieju commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartlomieju commented Oct 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bartlomieju commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ry left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bartlomieju commented Oct 13, 2021 •

edited

Loading

bartlomieju commented Oct 17, 2021 •

edited

Loading