refactor: integrate deno_graph 0.20.0 #13495

kitsonk · 2022-01-26T08:54:13Z

Down to 53 errors... just a chore to get the rest of them...

cli/Cargo.toml

kitsonk · 2022-01-29T21:37:24Z

Everything updates and compiles, but there are some regressions in the test cases that I need to dig into.

bartlomieju · 2022-01-30T02:43:52Z

I'm getting a lot of:

Don't know how to create cache name for scheme: flags
Don't know how to create cache name for scheme: flags

in tests. These originate in:

deno/cli/http_cache.rs

Line 45 in a2e4fa4

error!("Don't know how to create cache name for scheme: {}", scheme);

I feel like this should be an error rather than error log. If it happens it most likely means that we screwed up something in Deno internals.

This flags actually originates from here:

deno/cli/compat/mod.rs

Lines 85 to 86 in a2e4fa4

 static COMPAT_IMPORT_URL: Lazy<Url> = 

 Lazy::new(|| Url::parse("flags:compat").unwrap());

kitsonk · 2022-01-30T23:24:12Z

I'm getting a lot of:

Don't know how to create cache name for scheme: flags
Don't know how to create cache name for scheme: flags

Ok, yeah, I know the problem... Because we got rid of the seperate "Synthetic" modules, CLI is trying to cache all modules in the graph, including the synthetic ones. Need to filter those out.

kitsonk · 2022-01-31T01:18:39Z

@bartlomieju ok, I've got a few more challenges to fix, but there is one that I see what the problem is, but I am not totally sure why it is occurring now and the best way to fix it. If you run test native_modules_as_global_vars you will get a panic that the file being passed is not local. If you look at the stack trace, when the graph is being built, the node resolver is getting a request to resolve a remote URL and then is "blowing up". I believe the remote URL is being injected to the graph as part of the injected modules, but I am not totally sure why when building the graph, these injected dependencies weren't resolved before and why this isn't working now, nor why the compat code doesn't handle resolution of these "gracefully".

kitsonk · 2022-01-31T03:11:32Z

@bartlomieju it does look just like the compat failures with trying to find the package.json for the shim is the only outstanding issue.

bartlomieju · 2022-01-31T12:57:43Z

@bartlomieju ok, I've got a few more challenges to fix, but there is one that I see what the problem is, but I am not totally sure why it is occurring now and the best way to fix it. If you run test native_modules_as_global_vars you will get a panic that the file being passed is not local. If you look at the stack trace, when the graph is being built, the node resolver is getting a request to resolve a remote URL and then is "blowing up". I believe the remote URL is being injected to the graph as part of the injected modules, but I am not totally sure why when building the graph, these injected dependencies weren't resolved before and why this isn't working now, nor why the compat code doesn't handle resolution of these "gracefully".

@kitsonk there is a single path that injects resolved specifier for built-in Node modules (pointing to https://deno.land/std/node). Unfortunately I made a mistake when mapping these specifiers to ResolveResult. I now fixed it and the tests should pass. I think I will need to reorganize this code further when working on the CJS/ESM integration, because it is indeed unfortunate that there a single "special" code path that needs to handle http/https imports inside that resolution algorithm. Most likely it would be removed in the future if we decided to move std/node into an extension crate.

kitsonk · 2022-01-31T21:25:41Z

@bartlomieju obviously a topic for another day, but it would be really sad if we can't integrate Deno code and Node.js code in the same workload.

I fixed what I hope are the last fixture issues so the CI can go green.

dsherret

LGTM! Just one small fix needed though.

dsherret · 2022-01-31T21:35:24Z

cli/compat/esm_resolver.rs

@@ -121,9 +129,23 @@ fn node_resolve(
 let conditions = DEFAULT_CONDITIONS;
 let url = module_resolve(specifier, &parent_url, conditions)?;

+ let resolve_response = if url.as_str().starts_with("http") {


Nit: check url.scheme() instead?

@bartlomieju do you want to address or should I?

dsherret · 2022-01-31T21:37:33Z

cli/compat/esm_resolver.rs

@@ -121,9 +129,23 @@ fn node_resolve(
 let conditions = DEFAULT_CONDITIONS;
 let url = module_resolve(specifier, &parent_url, conditions)?;

+ let resolve_response = if url.as_str().starts_with("http") {
+ ResolveResponse::Esm(url)
+ } else if url.as_str().ends_with(".js") {


We should be doing case insensitive comparisons. It seems like we often forget about this so maybe we should create a helper function somewhere that does it efficiently and just in case, also checks url.path() instead of the full url?

@bartlomieju same again

dsherret · 2022-01-31T21:37:48Z

cli/compat/esm_resolver.rs

+ } else {
+ ResolveResponse::CommonJs(url)
+ }
+ } else if url.as_str().ends_with(".cjs") {


Here too (case insensitive, path comparison)

@bartlomieju same again

bartlomieju · 2022-01-31T22:17:26Z

@bartlomieju obviously a topic for another day, but it would be really sad if we can't integrate Deno code and Node.js code in the same workload.

I fixed what I hope are the last fixture issues so the CI can go green.

I don't think there's any problem with that - you can mix and match http/https imports with bare specifiers. I was mentioning situation with remapping of Node.js built in modules to std URLs.

bartlomieju

LGTM too. I'll apply David's suggestions in a follow up PR tomorrow morning. Let's land this!

kitsonk requested review from bartlomieju and dsherret as code owners January 26, 2022 08:54

kitsonk force-pushed the reaf_graph_0.20.0 branch from 0d36664 to e35ecd8 Compare January 28, 2022 05:39

dsherret reviewed Jan 28, 2022

View reviewed changes

cli/Cargo.toml Outdated Show resolved Hide resolved

kitsonk and others added 5 commits January 31, 2022 09:58

reafactor: integrate deno_graph 0.20.0

cd9020a

more

51f497c

just missing ast for dprint and lint

77e92f0

update deps

1fc20bd

update to latest changes in deno_graph

6ba6180

kitsonk force-pushed the reaf_graph_0.20.0 branch from 18d42b3 to 6ba6180 Compare January 30, 2022 22:58

fix cache info

c9ec9ad

other fixups

1488fca

kitsonk marked this pull request as draft January 31, 2022 03:01

bartlomieju and others added 5 commits January 31, 2022 13:58

fix ResolveResult for built-in Node modules

88bb5ce

Merge branch 'main' into reaf_graph_0.20.0

d8833bf

update Cargo.lock

35a14de

reset CI

8d688d2

fix fixtures on windows

c19c97e

kitsonk changed the title ~~[WIP] refactor: integrate deno_graph 0.20.0~~ refactor: integrate deno_graph 0.20.0 Jan 31, 2022

kitsonk requested a review from dsherret January 31, 2022 21:23

kitsonk marked this pull request as ready for review January 31, 2022 21:23

dsherret approved these changes Jan 31, 2022

View reviewed changes

one more fix for windows

b06f789

bartlomieju approved these changes Jan 31, 2022

View reviewed changes

kitsonk merged commit 7d35625 into denoland:main Jan 31, 2022

kitsonk deleted the reaf_graph_0.20.0 branch January 31, 2022 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: integrate deno_graph 0.20.0 #13495

refactor: integrate deno_graph 0.20.0 #13495

kitsonk commented Jan 26, 2022

kitsonk commented Jan 29, 2022

bartlomieju commented Jan 30, 2022

kitsonk commented Jan 30, 2022

kitsonk commented Jan 31, 2022

kitsonk commented Jan 31, 2022

bartlomieju commented Jan 31, 2022

kitsonk commented Jan 31, 2022

dsherret left a comment

dsherret Jan 31, 2022

kitsonk Jan 31, 2022

dsherret Jan 31, 2022 •

edited

Loading

kitsonk Jan 31, 2022

dsherret Jan 31, 2022

kitsonk Jan 31, 2022

bartlomieju commented Jan 31, 2022

bartlomieju left a comment

refactor: integrate deno_graph 0.20.0 #13495

refactor: integrate deno_graph 0.20.0 #13495

Conversation

kitsonk commented Jan 26, 2022

kitsonk commented Jan 29, 2022

bartlomieju commented Jan 30, 2022

kitsonk commented Jan 30, 2022

kitsonk commented Jan 31, 2022

kitsonk commented Jan 31, 2022

bartlomieju commented Jan 31, 2022

kitsonk commented Jan 31, 2022

dsherret left a comment

Choose a reason for hiding this comment

dsherret Jan 31, 2022

Choose a reason for hiding this comment

kitsonk Jan 31, 2022

Choose a reason for hiding this comment

dsherret Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

kitsonk Jan 31, 2022

Choose a reason for hiding this comment

dsherret Jan 31, 2022

Choose a reason for hiding this comment

kitsonk Jan 31, 2022

Choose a reason for hiding this comment

bartlomieju commented Jan 31, 2022

bartlomieju left a comment

Choose a reason for hiding this comment

dsherret Jan 31, 2022 •

edited

Loading