Add support for reloadSources, IsolateReload and PausePostRequest

[BlackHC]

This was tested with Dart 1.21.1. Remaining TODOs:

• semantic version update?
• Dart SDK constraints?
• tests for PausePostRequest and IsolateReload

PTAL! I don't know much about the remaining TODOs. Some advice would be awesome! :)

Thanks, Andreas

[BlackHC]

Reviewed 4 of 4 files at r1, 6 of 7 files at r3. Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

lib/src/isolate.dart, line 74 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

Explain what "reload" means in this context. Why is this returning a new isolate ref? Does a reload change the metadata at all? Also add a note indicating what protocol version this was added in.

All of this was added at protocol version 3.6 (unreleased atm?), see bottom of https://github.com/dart-lang/sdk/blob/ebe7d148f3964b17f1575a731e7d32e96855258d/runtime/vm/service/service.md I'm not sure how to treat this.

And good question. I don't think so. I've replaced it with Null now. This is purely if you want to listen to reload events.

---

lib/src/isolate.dart, line 332 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

Document this. In general, we try to make our documentation at least as good as the protocol documentation, and better where possible. Also add a note indicating what protocol version this was added in.

Done.

---

lib/src/isolate.dart, line 333 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

Name these "uri" parameters "url" instead. "Uri" is basically a deprecated term at this point.

Done.

---

lib/src/isolate.dart, line 335 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

Use `new ArgumentError.value()`.

Done. Pretty cool!

---

lib/src/reload_report.dart, line 1 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

2017

Done.

---

lib/src/reload_report.dart, line 11 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

This comment isn't accurate.

Done.

---

lib/src/reload_report.dart, line 12 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

This needs to be exported from `lib/vm_service_client.dart`.

Done.

---

test/isolate_test.dart, line 76 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

We don't really use tags in this package, so tagging only this test is strange. In general, I don't use tags unless I have a specific reason to—either wanting to select a subset of tests that appear across many different files, or if I want to add special metadata for that tag.

Done. Sorry.

---

lib/src/pause_event.dart, line 107 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

It looks like this isn't being constructed anywhere...

Done.

---

Comments from Reviewable

[BlackHC]

Thanks for the review PTAL. The test to check that onReload works actually surfaced an issue. It only triggers for failed reloads, not for successful ones. I've filed an issue at https://github.com/dart-lang/sdk/issues/28960.

How shall we proceed?

Thanks, Andreas

[nex3]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

lib/src/isolate.dart, line 74 at r1 (raw file):

I'm not sure how to treat this.

Your change looks good.

I've replaced it with Null now.

This package doesn't use <Null>. I generally like following the spec-mode convention of leaving off the type annotation, at least until the strong mode semantics of <Null> and <void> are sorted out for good.

---

lib/src/isolate.dart, line 332 at r1 (raw file):

Previously, BlackHC (Andreas Kirsch) wrote…

Done.

It's still not really clear what it means to "reload sources". Which sources are affected? What changes? Does it affect already-instantiated objects or only new ones?

---

test/isolate_test.dart, line 77 at r3 (raw file):

      // right now, issue tracked at:
      // https://github.com/dart-lang/sdk/issues/28960
      final report = await isolate.reloadSources(force: true);

Style nit: use var for local variables.

---

test/isolate_test.dart, line 79 at r3 (raw file):

      final report = await isolate.reloadSources(force: true);
      expect(report.status, true);
    });

This should also check that it actually reloads the sources—construct a newly-added class or something.

Also, make sure to test each parameter.

---

test/isolate_test.dart, line 87 at r3 (raw file):

      // TODO(blackhc): IsolateReload provides more information which we do
      // not surface currently. We should verify the error type, too.
      expect(isolate.onReload.first, completion(isNull));

It might be nice to verify that this only fires once. Using test's new stream matchers I think you can write expect(isolate.onReload, emitsInOrder([isNull, neverEmits(anything)])).

---

test/isolate_test.dart, line 110 at r3 (raw file):

      var queue = new StreamQueue(main.onPauseOrResume);
      expect(
          queue.next, completion(new isInstanceOf<VMPauseInterruptedEvent>()));

Please remove unrelated formatting changes.

---

Comments from Reviewable

[BlackHC]

Hey! I've run into some other things that I need for the REPL that are undocumented in the spec, so I don't feel like I can add them to the library/this CL. I can't prioritize the review over just using it and finishing the next iteration. Could you take over the remainder of the CL maybe? I won't be able to look at this in the near future :( I'm just going to use a separate poc git branch for now. Sorry!! :(

Thanks, Andreas

---

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

lib/src/isolate.dart, line 332 at r1 (raw file):

Previously, nex3 (Natalie Weizenbaum) wrote…

It's still not really clear what it means to "reload sources". Which sources are affected? What changes? Does it affect already-instantiated objects or only new ones?

To be honest, I don't really know myself. This is what's explained in the VM service spec :(

---

Comments from Reviewable

[nex3]

I don't currently have time to put into this either, but I'll leave it open for now until I do or until someone else wants to adopt it.