refactor: use Url in deno dir #2572

bartlomieju · 2019-06-23T13:48:39Z

Follow-up to #2559 (and blocked by it)

Use Url instead of String for specifiers in //cli/deno_dir.rs

cli/deno_dir.rs

bartlomieju · 2019-06-24T09:52:23Z

Note to self: our current resolving of paths (including ModuleSpecifier::resolve_root) is not 100% correct.

These specifiers are not resolved properly on windows:

// 1.
C:\deno\tests\006_url_imports.ts 
// result URL 
C:\\deno\\tests\\006_url_imports.ts
// expected URL
file:///C:/deno/tests/006_url_imports.ts

// this one fails completely
\deno\tests\006_url_imports.ts

Consider using NormalizedPath instead of Path that leverages normalize_path from deno_dir.rs - this will ensure that all paths passed around our code base use / (forward slash) instead of mixing forward and backward slashes. This is fine, because fs module can handle forward slashes on Windows.

bartlomieju · 2019-06-28T15:30:34Z

Rebased on top of #2581

cli/deno_dir.rs

bartlomieju · 2019-06-29T10:53:10Z

@ry @kitsonk please review - this PR is meant to replace use of &str in favor of &Url in DenoDir. There are still some places where this should be changed (eg. ModuleMetaData uses module_name which has String type that is actually Url casted to string), but this PR is quite complex and touching crucial stuff already.

During work on this PR I've got pretty good idea how to rewrite DenoDir, but I'll write up my thoughts in appropriate issue. @kitsonk I'd very much would like to help you out during rewrite of you'd want.

cli/deno_dir.rs

ry · 2019-06-29T14:40:12Z

cli/deno_dir.rs

-    match self.fetch_module_meta_data(script_name, true, true) {
+    // TODO: this method shouldn't issue `fetch_module_meta_data` - this is done for each line
+    //  in JS stack trace so it's pretty slow - quick idea: store `ModuleMetaData` in one
+    //  structure available to DenoDir so it's not fetched from disk everytime it's needed


cli/deno_dir.rs

ry · 2019-06-29T14:47:42Z

@bartlomieju This is a great patch and it looks good except for a few nits

During work on this PR I've got pretty good idea how to rewrite DenoDir, but I'll write up my thoughts in appropriate issue

Great. I look forward to it.

cli/deno_dir.rs

core/module_specifier.rs

bartlomieju · 2019-06-30T19:38:27Z

@piscisaureus I merged latest master

bartlomieju · 2019-06-30T20:03:49Z

core/module_specifier.rs

+      UnsupportedPathPrefix => write!(
+        f,
+        r"invalid path prefix, must be disk prefix (eg. C:/, C:\\)"
+      ),


Messages for these two variants should print out specifier and path respectively.

ImportPathPrefixMissing(ref specifier) => { write!(f, "Invalid import path: \"{}\". Relative import must be prefixed with / or ./ or ../", specifier.to_str().unwrap()) } UnsupportedPathPrefix(ref path) => { write!( f, "Unsupported path prefix: \"{}\". Must be disk prefix (eg. C:/, C:\\\\)", path.to_str().unwrap() ) },

Trying to figure it out, but neither String nor PathBuf implement Copy trait... 🤔

ry · 2019-07-01T13:39:28Z

LGTM from me.

@piscisaureus - approve and land when you're satisfied.

piscisaureus · 2019-07-02T15:42:59Z

Still waiting for my windows machine. ETA now: tomorrow. Hang in there.

bartlomieju · 2019-07-04T17:59:29Z

@piscisaureus any update?

piscisaureus · 2019-07-06T17:11:47Z

I'm actually not so happy with the logic that decides whether a command-line argument is a path or a URL.
Especially since this requires encoding a lot of knowledge about windows path formats, and the heuristic that's uses is complex and hard to describe.

What I would suggest is the following

If the argument starts with something that looks like the 'scheme' part of a URL (it matches /^[a-z][a-z+-\.]+:/i`), treat it as a URL.
Otherwise, treat it as a path; make it absolute first then convert to a file URL.

With that you can remove all windows-specific path handling and let rust-std deal with it, which seems to do it well.

bartlomieju · 2019-07-06T17:19:53Z

What I would suggest is the following

If the argument starts with something that looks like the 'scheme' part of a URL (it matches /^[a-z][a-z+-\.]+:/i`), treat it as a URL.

Otherwise, treat it as a path; make it absolute first then convert to a file URL.

With that you can remove all windows-specific path handling and let rust-std deal with it, which seems to do it well.

@piscisaureus that is exactly what happens at the moment. The only Windows-specific thing is ensure_valid_prefix which allows only disk prefixes that you suggested.

piscisaureus · 2019-07-07T23:40:14Z

Sorry, but it's really not right. Take this change for example:

  /// Resolves module using this algorithm:
  /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
  pub fn resolve(
    specifier: &str,
    base: &str,
  ) -> Result<ModuleSpecifier, ModuleResolutionError> {
    use ModuleResolutionError::*;

    // 1. Apply the URL parser to specifier. If the result is not failure, return
    //    the result.
    if let Ok(module_specifier) = ModuleSpecifier::resolve_absolute(specifier) {
      return Ok(module_specifier);
    }

    ...
}

ModuleSpecifier::resolve_absolute will first try to interpret the specifier as a path, and then fall back to parsing it as a URL only if that fails.
That's in direct contradiction of the spec.

piscisaureus · 2019-07-07T23:41:03Z

I'll try to pump out a patch tonight that does it correctly.

bartlomieju · 2019-07-08T09:27:01Z

core/module_specifier.rs

+  /// We additionally require the scheme to be at least 2 characters long,
+  /// because otherwise a windows path like c:/foo would be treated as a URL,
+  /// while no schemes with a one-letter name actually exist.
+  fn specifier_has_uri_scheme(specifier: &str) -> bool {


I was gonna argue that trying path first before parsing URL was to handle Windows path prefixed with disk, but as always your solution is much more elegant 👍 kudos @piscisaureus

piscisaureus · 2019-07-08T09:49:29Z

@ry, @bartlomieju
I re-did the changes to the ModuleSpecifier resolution logic. PTAL at 9b1997b.
If that's alright with you guys then I think we can land this PR.

The rules are now as follows: * In `import` statements, as mandated by the WHATWG specification, the import specifier is always treated as a URL. If it is a relative URL, it must start with either / or ./ or ../ * A script name passed to deno as a command line argument may be either an absolute URL or a local path. - If the name starts with a valid URI scheme followed by a colon, e.g. 'http:', 'https:', 'file:', 'foo+bar:', it always interpreted as a URL (even if Deno doesn't support the indicated protocol). - Otherwise, the script name is interpreted as a local path. The local path may be relative, and operating system semantics determine how it is resolved. Prefixing a relative path with ./ is not required.

piscisaureus · 2019-07-08T11:13:42Z

core/module_specifier.rs

+        "file:///home/yeti/deno",
+        "file://server/some/dir/file",
+      ),
+      // This test is disabled because the url crate does not follow the spec,


servo/rust-url#504

bartlomieju · 2019-07-08T12:06:35Z

@piscisaureus LGTM. I like your solution to ModuleSpecifier 👍

bartlomieju commented Jun 23, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

bartlomieju force-pushed the refactor-use_url_in_deno_dir branch 6 times, most recently from b23c971 to d52d870 Compare June 23, 2019 22:03

bartlomieju mentioned this pull request Jun 24, 2019

refactor: use Path/PathBuf in deno dir #2559

Merged

bartlomieju force-pushed the refactor-use_url_in_deno_dir branch from 4e2008e to e9efe59 Compare June 24, 2019 17:45

bartlomieju force-pushed the refactor-use_url_in_deno_dir branch from e9efe59 to e02d816 Compare June 28, 2019 15:28

bartlomieju force-pushed the refactor-use_url_in_deno_dir branch 2 times, most recently from 3367275 to 2f063ec Compare June 28, 2019 21:12

bartlomieju commented Jun 29, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

bartlomieju changed the title ~~WIP refactor: use Url in deno dir~~ refactor: use Url in deno dir Jun 29, 2019

ry reviewed Jun 29, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

ry reviewed Jun 29, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

ry reviewed Jun 29, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

bartlomieju commented Jun 29, 2019

View reviewed changes

cli/deno_dir.rs Outdated Show resolved Hide resolved

bartlomieju commented Jun 29, 2019

View reviewed changes

core/module_specifier.rs Show resolved Hide resolved

bartlomieju commented Jun 29, 2019

View reviewed changes

core/module_specifier.rs Outdated Show resolved Hide resolved

bartlomieju commented Jun 30, 2019

View reviewed changes

ry approved these changes Jul 1, 2019

View reviewed changes

piscisaureus force-pushed the refactor-use_url_in_deno_dir branch 9 times, most recently from 562e61d to d034ff4 Compare July 8, 2019 09:10

bartlomieju commented Jul 8, 2019

View reviewed changes

piscisaureus force-pushed the refactor-use_url_in_deno_dir branch from d034ff4 to d0447c6 Compare July 8, 2019 09:48

piscisaureus force-pushed the refactor-use_url_in_deno_dir branch from dd42e3f to d0447c6 Compare July 8, 2019 10:29

piscisaureus and others added 2 commits July 8, 2019 13:07

cli: refactor deno_dir to use Url instead of String

72d9045

piscisaureus force-pushed the refactor-use_url_in_deno_dir branch from d0447c6 to 72d9045 Compare July 8, 2019 11:07

piscisaureus reviewed Jul 8, 2019

View reviewed changes

piscisaureus merged commit 72d9045 into denoland:master Jul 8, 2019

bartlomieju deleted the refactor-use_url_in_deno_dir branch July 8, 2019 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use Url in deno dir #2572

refactor: use Url in deno dir #2572

bartlomieju commented Jun 23, 2019

bartlomieju commented Jun 24, 2019

bartlomieju commented Jun 28, 2019

bartlomieju commented Jun 29, 2019

ry Jun 29, 2019

ry commented Jun 29, 2019

bartlomieju commented Jun 30, 2019

bartlomieju Jun 30, 2019 •

edited

Loading

ry commented Jul 1, 2019

piscisaureus commented Jul 2, 2019

bartlomieju commented Jul 4, 2019

piscisaureus commented Jul 6, 2019

bartlomieju commented Jul 6, 2019

piscisaureus commented Jul 7, 2019

piscisaureus commented Jul 7, 2019

bartlomieju Jul 8, 2019

piscisaureus commented Jul 8, 2019 •

edited

Loading

piscisaureus Jul 8, 2019

bartlomieju commented Jul 8, 2019

refactor: use Url in deno dir #2572

refactor: use Url in deno dir #2572

Conversation

bartlomieju commented Jun 23, 2019

bartlomieju commented Jun 24, 2019

bartlomieju commented Jun 28, 2019

bartlomieju commented Jun 29, 2019

ry Jun 29, 2019

Choose a reason for hiding this comment

ry commented Jun 29, 2019

bartlomieju commented Jun 30, 2019

bartlomieju Jun 30, 2019 • edited Loading

Choose a reason for hiding this comment

ry commented Jul 1, 2019

piscisaureus commented Jul 2, 2019

bartlomieju commented Jul 4, 2019

piscisaureus commented Jul 6, 2019

bartlomieju commented Jul 6, 2019

piscisaureus commented Jul 7, 2019

piscisaureus commented Jul 7, 2019

bartlomieju Jul 8, 2019

Choose a reason for hiding this comment

piscisaureus commented Jul 8, 2019 • edited Loading

piscisaureus Jul 8, 2019

Choose a reason for hiding this comment

bartlomieju commented Jul 8, 2019

bartlomieju Jun 30, 2019 •

edited

Loading

piscisaureus commented Jul 8, 2019 •

edited

Loading