Skip to content

Account for server root #686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 8, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
11 changes: 9 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,11 @@ class Sources {
for (var entry in lineEntry.entries) {
var index = entry.sourceUrlId;
if (index == null) continue;
var dartUri = DartUri(mapping.urls[index], script.url);
// TODO(grouma) - This work seems expensive and likely should be
// cached.
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
44 changes: 27 additions & 17 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,34 @@ 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('/')) return DartUri._fromServerPath(uri);
if (uri.startsWith('/packages/')) {
return DartUri._fromRelativePath(uri, serverUri: serverUri);
}
if (uri.startsWith('/')) return DartUri._fromRelativePath(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) => p.dirname(Uri.parse(uri).path);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


/// 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._fromRelativePath(
p.join(_dirForServerUri(serverUri), packagePath));
}
return DartUri._(packagePath);
}

/// Construct from a file: URI
Expand All @@ -138,17 +144,21 @@ class DartUri {
factory DartUri._fromAppUri(String uri) {
// We ignore the first segment of the path, which is the root
// from which we're serving.
// TODO: To be able to convert to an org-dartlang-app: URI we will
// need to know the root - possibly keep it as a static?
return DartUri._(Uri.parse(uri).pathSegments.skip(1).join('/').toString());
var path = 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._fromRelativePath(String uri, {String serverUri}) {
uri = uri[0] == '.' ? uri.substring(1) : uri;
uri = uri[0] == '/' ? uri.substring(1) : uri;

if (serverUri != null) {
return DartUri._fromRelativePath(
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