Account for server root #686

grouma · 2019-09-26T20:19:38Z

No description provided.

alan-knight · 2019-09-26T20:30:05Z

dwds/lib/src/utilities/dart_uri.dart

    }
    throw FormatException('Unsupported URI form', uri);
  }

+  static String _dirForServerUri(String uri) => p.dirname(Uri.parse(uri).path);


This probably needs a comment.

alan-knight · 2019-09-26T20:34:31Z

dwds/lib/src/utilities/dart_uri.dart

-      var dir = p.dirname(path);
-      return DartUri._fromServerPath(p.normalize(p.join(dir, uri)));
+      return DartUri._fromServerPath(
+          p.normalize(p.join(_dirForServerUri(serverUri), uri)));


Why do we need the normalize here? If uri is coming from an http URI, and serverUri is the prefix path on the server, there should never be . or .., right?

We shouldn't need to

alan-knight · 2019-09-26T20:35:48Z

dwds/lib/src/utilities/dart_uri.dart

@@ -98,7 +98,9 @@ class DartUri {
  /// JS script. The dirname of that path should give us the missing prefix.
  factory DartUri(String uri, [String serverUri]) {
    // TODO(401): Remove serverUri after D24 is stable.


Remove this if we're using serverUri for something else.

alan-knight · 2019-09-26T20:36:00Z

dwds/lib/src/utilities/dart_uri.dart

@@ -109,16 +111,22 @@ class DartUri {
    }
    // Work around short paths if we have been provided the context.


This doesn't apply any more.

alan-knight · 2019-09-26T20:37:20Z

dwds/lib/src/utilities/dart_uri.dart

@@ -109,16 +111,22 @@ class DartUri {
    }
    // Work around short paths if we have been provided the context.
    if (serverUri != null) {


Actually I think this should never happen now. This is the case where we get a raw path, not a URL with a scheme. I think that shouldn't be happening any more and this can be deleted

That's not true. We still get URI's like main.dart that are not relative but have the corresponding context.

alan-knight · 2019-09-26T20:38:11Z

dwds/lib/src/utilities/dart_uri.dart

  /// Construct from a package: URI
-  factory DartUri._fromPackageUri(String uri) {
-    return DartUri._('packages/${uri.substring("package:".length)}');
+  factory DartUri._fromPackageUri(String uri, String serverUri) {


Why does this apply to package: URIs, but not org-dartlang-app: URIs?

We discussed that we probably need to account for the serverUri to properly work with proxy servers. Updated the logic.

jakemac53 · 2019-10-07T21:04:29Z

dwds/CHANGELOG.md

@@ -1,3 +1,6 @@
+## 0.7.2
+- Account for root directory path when using `package:` URIs with `DartUri`.


nit: newline between for consistency

jakemac53 · 2019-10-07T21:06:08Z

dwds/lib/src/utilities/dart_uri.dart

  /// Construct from a package: URI
-  factory DartUri._fromPackageUri(String uri) {
-    return DartUri._('packages/${uri.substring("package:".length)}');
+  factory DartUri._fromPackageUri(String uri, String serverUri) {


if serverUri is nullable I would make it an optional (named) arg (ditto below)

jakemac53 · 2019-10-07T21:07:50Z

dwds/test/dart_uri_test.dart

@@ -17,6 +17,12 @@ void main() {
      expect(uri.serverPath, 'packages/path/path.dart');
    });

+    test('parses package : paths with root', () {
+      var uri = DartUri(
+          'package:path/path.dart', 'http://localhost:8080/foo/bar/blah');


I am a bit concerned about how the hardcoded knowledge to walk up one directory here

Yeah. This may cause issues in the future, especially with proxy servers but this is what is expected today.

grouma · 2019-10-07T23:48:18Z

Discussed offline with @alan-knight. Made some changes to account for source maps URLs. This should be ready to review. PTAL.

alan-knight · 2019-10-07T23:53:54Z

dwds/lib/src/debugging/debugger.dart

@@ -169,7 +169,7 @@ class Debugger extends Domain {
  }

  Future<Null> _initialize() async {
-    sources = Sources(_assetHandler, _remoteDebugger, _logWriter);
+    sources = Sources(_assetHandler, _remoteDebugger, _logWriter, _root);


Maybe we could have the root URI accessible somewhere instead of passing it around to different objects?

That's probably a good clean up that should follow this PR.

alan-knight · 2019-10-07T23:55:37Z

dwds/lib/src/debugging/sources.dart

@@ -77,7 +80,9 @@ class Sources {
        for (var entry in lineEntry.entries) {
          var index = entry.sourceUrlId;
          if (index == null) continue;
-          var dartUri = DartUri(mapping.urls[index], script.url);
+          var path = p.join(
+              p.dirname(Uri.parse(script.url).path), mapping.urls[index]);


I feel like Uri.parse is expensive and it would be good if we could avoid doing it in this loop. But maybe file an optimization bug to cache it or otherwise optimize.

alan-knight · 2019-10-07T23:57:12Z

dwds/lib/src/utilities/dart_uri.dart

    if (uri.startsWith('org-dartlang-app:')) return DartUri._fromAppUri(uri);
    if (uri.startsWith('google3:')) return DartUri._fromGoogleUri(uri);
    if (uri.startsWith('file:')) return DartUri._fromFileUri(uri);
-    if (uri.startsWith('/packages/')) return DartUri._fromServerPath(uri);
+    if (uri.startsWith('/packages/')) {
+      return DartUri._fromServerPath(uri, serverUri: serverUri);


Should this be something like _fromRelativePath?

alan-knight · 2019-10-07T23:57:50Z

dwds/lib/src/utilities/dart_uri.dart

@@ -138,17 +146,22 @@ class DartUri {
  factory DartUri._fromAppUri(String uri) {
    // We ignore the first segment of the path, which is the root
    // from which we're serving.
+    var path = Uri.parse(uri).pathSegments.skip(1).join('/').toString();
    // TODO: To be able to convert to an org-dartlang-app: URI we will


I don't think this comment is valid any more.

alan-knight · 2019-10-07T23:58:22Z

dwds/pubspec.yaml

@@ -1,5 +1,5 @@
 name: dwds
-version: 0.7.1
+version: 0.7.2


Should this be -dev, or are you planning to publish right away?

I intend to publish right away.

Account for server root

c7ea09e

grouma requested a review from alan-knight September 26, 2019 20:19

alan-knight reviewed Sep 26, 2019

View reviewed changes

grouma added 2 commits October 7, 2019 12:44

Merge branch 'master' into account-for-root

dbf57a6

respond to review

c576c91

grouma requested a review from jakemac53 October 7, 2019 20:56

jakemac53 reviewed Oct 7, 2019

View reviewed changes

grouma added 3 commits October 7, 2019 15:01

make source map path clear

4b5ea01

optional arg

1b10630

fix tests and account for source maps

3ee0991

don't worry about leading slash

e4412aa

alan-knight approved these changes Oct 7, 2019

View reviewed changes

clean up

6fc70d5

grouma merged commit 65fd8e1 into master Oct 8, 2019

grouma deleted the account-for-root branch October 8, 2019 00:35

		@@ -109,16 +111,22 @@ class DartUri {
		}
		// Work around short paths if we have been provided the context.

		@@ -1,3 +1,6 @@
		## 0.7.2
		- Account for root directory path when using `package:` URIs with `DartUri`.

Account for server root #686

Account for server root #686

Uh oh!

Conversation

grouma commented Sep 26, 2019

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!

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!

grouma commented Oct 7, 2019

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!