Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## 0.7.2
- Account for root directory path when using `package:` URIs with `DartUri`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline between for consistency


## 0.7.1

- Fix a bug where we would try to create a new isolate even for a failed
Expand Down
39 changes: 28 additions & 11 deletions dwds/lib/src/utilities/dart_uri.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

import 'dart:io';

import 'package:path/path.dart' as p;
import 'package:package_resolver/package_resolver.dart';
import 'package:path/path.dart' as p;

/// The URI for a particular Dart file, able to canonicalize from various
/// different representations.
Expand Down Expand Up @@ -97,28 +97,41 @@ class DartUri {
/// packages/path/src/path.dart. The optional [serverUri] is the full URI of the
/// 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.
if (uri.startsWith('package:')) return DartUri._fromPackageUri(uri);
if (uri.startsWith('org-dartlang-app:')) return DartUri._fromAppUri(uri);
if (uri.startsWith('package:')) {
return DartUri._fromPackageUri(uri, serverUri);
}
if (uri.startsWith('org-dartlang-app:')) {
return DartUri._fromAppUri(uri, serverUri);
}
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('/')) return DartUri._fromServerPath(uri);
if (uri.startsWith('http:') || uri.startsWith('https:')) {
return DartUri(Uri.parse(uri).path);
}

// Work around short paths if we have been provided the context.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't apply any more.

if (serverUri != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var path = Uri.parse(serverUri).path;
var dir = p.dirname(path);
return DartUri._fromServerPath(p.normalize(p.join(dir, uri)));
return DartUri._fromServerPath(
p.normalize(p.join(_dirForServerUri(serverUri), uri)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to

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

/// Returns the dirname for the server URI.
static String _dirForServerUri(String uri) =>
// Path will be of the form `/foo/bar` so ignore the leading `/`.
p.dirname(Uri.parse(uri).path.substring(1));

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var packagePath = 'packages/${uri.substring("package:".length)}';
if (serverUri != null) {
return DartUri._fromServerPath(
p.join(_dirForServerUri(serverUri), packagePath));
}
return DartUri._(packagePath);
}

/// Construct from a file: URI
Expand All @@ -135,12 +148,16 @@ class DartUri {
}

/// Construct from an org-dartlang-app: URI.
factory DartUri._fromAppUri(String uri) {
factory DartUri._fromAppUri(String uri, String serverUri) {
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// need to know the root - possibly keep it as a static?
return DartUri._(Uri.parse(uri).pathSegments.skip(1).join('/').toString());
if (serverUri != null) {
return DartUri._(p.normalize(p.join(_dirForServerUri(serverUri), path)));
}
return DartUri._(path);
}

DartUri._(this.serverPath);
Expand Down
2 changes: 1 addition & 1 deletion dwds/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: dwds
version: 0.7.1
version: 0.7.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to publish right away.

author: Dart Team <[email protected]>
homepage: https://github.com/dart-lang/webdev/tree/master/dwds
description: >-
Expand Down
6 changes: 6 additions & 0 deletions dwds/test/dart_uri_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expect(uri.serverPath, 'foo/bar/packages/path/path.dart');
});

test('parses org-dartlang-app paths', () {
var uri = DartUri('org-dartlang-app:////blah/main.dart');
expect(uri.serverPath, 'blah/main.dart');
Expand Down
4 changes: 4 additions & 0 deletions webdev/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.5.3-dev

- Depend on the latest `package:dwds`.

## 2.5.2

- Update SDK dependency to minimum of 2.5.0.
Expand Down
2 changes: 1 addition & 1 deletion webdev/lib/src/version.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion webdev/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: webdev
# Every time this changes you need to run `pub run build_runner build`.
version: 2.5.2
version: 2.5.3-dev
author: Dart Team <[email protected]>
homepage: https://github.com/dart-lang/webdev
description: >-
Expand Down Expand Up @@ -46,3 +46,7 @@ dev_dependencies:

executables:
webdev:

dependency_overrides:
dwds:
path: ../dwds