Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
4 changes: 4 additions & 0 deletions dwds/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 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
2 changes: 1 addition & 1 deletion dwds/lib/src/debugging/debugger.dart
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ class Debugger extends Domain {
}

Future<Null> _initialize() async {
sources = Sources(_assetHandler, _remoteDebugger, _logWriter);
sources = Sources(_assetHandler, _remoteDebugger, _logWriter, _root);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 probably a good clean up that should follow this PR.

// We must add a listener before enabling the debugger otherwise we will
// miss events.
// Allow a null debugger/connection for unit tests.
Expand Down
9 changes: 7 additions & 2 deletions dwds/lib/src/debugging/sources.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ class Sources {

final RemoteDebugger _remoteDebugger;

Sources(this._assetHandler, this._remoteDebugger, this._logWriter);
final String _root;

Sources(
this._assetHandler, this._remoteDebugger, this._logWriter, this._root);

/// Returns all [Location] data for a provided Dart source.
Set<Location> locationsForDart(String serverPath) =>
Expand Down Expand Up @@ -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]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

var dartUri = DartUri(path, _root);
var location = Location.from(
script.scriptId,
lineEntry,
Expand Down
41 changes: 27 additions & 14 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,36 @@ 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('package:')) {
return DartUri._fromPackageUri(uri, serverUri: serverUri);
}
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);
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 something like _fromRelativePath?

}
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.
if (serverUri != null) {
var path = Uri.parse(serverUri).path;
var dir = p.dirname(path);
return DartUri._fromServerPath(p.normalize(p.join(dir, uri)));
}

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}) {
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 @@ -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
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());
return DartUri._(path);
}

DartUri._(this.serverPath);

/// Construct from a path, relative to the directory being served.
factory DartUri._fromServerPath(String uri) {
factory DartUri._fromServerPath(String uri, {String serverUri}) {
uri = uri[0] == '.' ? uri.substring(1) : uri;
uri = uri[0] == '/' ? uri.substring(1) : uri;

if (serverUri != null) {
return DartUri._fromServerPath(p.join(_dirForServerUri(serverUri), uri));
}
return DartUri._(uri);
}
}
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
11 changes: 6 additions & 5 deletions dwds/test/dart_uri_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@ import 'package:test/test.dart';

void main() {
group('DartUri', () {
test('normalizes server paths', () {
var uri = DartUri('../foo.dart', '/packages/blah/src/blah.dart');
expect(uri.serverPath, 'packages/blah/foo.dart');
});

test('parses package : paths', () {
var uri = DartUri('package:path/path.dart');
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
2 changes: 1 addition & 1 deletion dwds/test/debugging/sources_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ void main() {
};
var logs = <LogRecord>[];
var sources = Sources(TestingAssetHandler(assets), null,
(level, message) => logs.add(LogRecord(level, message, '')));
(level, message) => logs.add(LogRecord(level, message, '')), '');
var serverUri = 'http://localhost:1234/';
await sources.scriptParsed(ScriptParsedEvent(WipEvent({
'params': {
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