-
Notifications
You must be signed in to change notification settings - Fork 79
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
Reorganized package, put example it it's own directory, etc #9
Conversation
d1f5a81
to
cfe2534
Compare
@natebosch @nshahan – this is a good start, I think – PTAL |
var exitPort = new ReceivePort(); | ||
await Isolate.spawnUri(await _buildRunnerScript(), arguments, null, | ||
onExit: exitPort.sendPort, automaticPackageResolution: true); | ||
await exitPort.first; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an FYI this isn't guaranteed to fire.
See all the extra handling we have for exception cases: https://github.com/dart-lang/build/blob/0c77443dd74edda706e89189c5ccfb70b06a22d1/build_runner/bin/build_runner.dart#L58-L85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There oughta be a helper...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a todo? File an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes there should :)
I still haven't made up my mind whether I want a specific helper in build_runner
or whether this we should make a more user friendly API around isolates in package:io
… On Mon, Mar 19, 2018 at 3:09 PM Nate Bosch ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In webdev/lib/src/command/build_runner_command_base.dart
<#9 (comment)>:
> @@ -19,13 +24,66 @@ abstract class BuildRunnerCommandBase extends Command {
help: 'Enables verbose logging.');
}
- Future<Uri> get buildRunnerScript async {
- // TODO(nshahan) build_runner will expose this as a function call that will
- // be imported to avoid running a binary in a transitive dependency with
- // pub run.
- final executable = 'pub';
- final arguments = ['run', 'build_runner', 'generate-build-script'];
- final results = await Process.run(executable, arguments);
- return new Uri.file(results.stdout.toString().trim());
+ Future runCore(String command) async {
+ final arguments = [command]..addAll(argResults.arguments);
+ var exitPort = new ReceivePort();
+ await Isolate.spawnUri(await _buildRunnerScript(), arguments, null,
+ onExit: exitPort.sendPort, automaticPackageResolution: true);
+ await exitPort.first;
yes there should :)
I still haven't made up my mind whether I want a specific helper in
build_runner or whether this we should make a more user friendly API
around isolates in package:io
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABCirkj2px1G3ssVhTmm87jv5_9hDXcks5tgCyNgaJpZM4SuXxl>
.
|
dev_dependencies: | ||
test: "^0.12.0" | ||
executables: | ||
webdev: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm - we don't plan on publishing this until we've implemented the upper bound version check on build_runner right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, not ready to publish until we have locked down the validation parts to make sure people don't get a rough introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
Yup!
…On Mon, Mar 19, 2018, 15:38 Nate Bosch ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In webdev/pubspec.yaml
<#9 (comment)>:
>
-dev_dependencies:
- test: "^0.12.0"
+executables:
+ webdev:
just to confirm - we don't plan on publishing this until we've implemented
the upper bound version check on build_runner right?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABCiqXxO-_bMzhVwZhfyn_WrTx0833kks5tgDNIgaJpZM4SuXxl>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments but otherwise LGTM
dev_dependencies: | ||
test: "^0.12.0" | ||
executables: | ||
webdev: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, not ready to publish until we have locked down the validation parts to make sure people don't get a rough introduction.
example/pubspec.yaml
Outdated
sdk: ">=2.0.0-dev.32.0 <2.0.0" | ||
|
||
dependencies: | ||
angular: ^5.0.0-alpha+3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we bump to alpha+8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh...sure
No description provided.