- 
                Notifications
    You must be signed in to change notification settings 
- Fork 221
Adding incremental Frontend Server-compatible DDC builders + supporting infrastructure #4240
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
base: master
Are you sure you want to change the base?
Conversation
…r fine modules for hot reload
| PR HealthChangelog Entry ✔️
 Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with  | 
| Package publishing
 Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. | 
| @davidmorgan Wow, the tests in  Thanks for the review - back from vacation! | 
| It looks like you made some changes in response to my comments but I don't see any responses to the comments so I'm not sure if you're done--please let me know if/when I should take another look. Thanks! | 
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.
Welcome back!
Looking good so far.
There are some CI failures, could you take a look at those please?
| /// defaults to [BuildLog.failurePattern] so that `expect` will stop if the | ||
| /// process reports a build failure. | ||
| /// | ||
| /// if [expectFailure] is set, then both [pattern] and [failOn] must be | 
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.
Hmmmm multiple expectations is a bit awkward, I wonder if there's a nicer way.
Would it work to accumulate lines and return them, then the second expectation can be checked on the result?
Looking at how this method is used, I think maybe something like this:
- most callers are not using the result, so how about the base method is Future<void> expect.
- a few callers are using the single matching line, so how about Future<String> expectAndGetLinefor that
- and then I think you can do what you want with the full accumulated output until the match, how about Future<List<String>> expectAndGetBlock
?
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 like that change, but I the newly added tests' reqs are:
- we should be able to expect that a test throws
- we should be able to match on more complex patterns (more than one line of match)
The problem with returning a block is that expect waits on a stream until it times out for 30s or encounters pattern, so we don't have a good way to determine when a block is finished accumulating. Really it should just accept a List<Matcher> and return if all matchers are successful. Thoughts?
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.
When you say "test throws" do you mean "logs an error"? It's supposed to be that no exceptions escape :)
build_runner should always run the build to completion then either print "build succeeded" or "build failed", so it should always work to use one of those or a RegExp that matches either to capture a block.
If this doesn't work could you please point to a specific test so I can understand? Thanks.
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.
Yep, logs an error (i.e., "build failed"). Hmm, watcher tests appear to stream the results of a build until some string or Regexp is matched - line by line. I'd like to both match specific text of a line and match the "build failed" text (which is output on a later line), which isn't supported by the current testing scheme (unless I've missed something). The 'can recompile incrementally after invalid edits' test is an example of this. await watch.expect(BuildLog.successPattern);, which seems to be the canonical failure detecting check, doesn't let me check the error text.
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.
So currently you have this
    await watch.expect(
      'Hot reload rejected due to unsupported changes',
      expectFailure: true,
    );
I think you could add a new method expectAndGetBlock, that was the name given above, but maybe linesUntil would be better:
  final block = await watch.linesUntil(BuildLog.failurePattern);
  expect(block, contains(contains('Hot reload rejected due to unsupported changes')));
?
Per my original comment I think "expect" could be three versions: one that returns nothing, one that returns the matched line, and one that returns all the lines. Or maybe drop the single line version, you can always just do ".last" on the block?
| 
 Whoops, looks like I had unpublished comments that you addressed by coincidence. Sorry about that! Now published. | 
| 
 @davidmorgan Oh no - you didn't see my comments? I can ping you when things are fully ready, but I've been in and out since I'm sick with something mysterious lol @nshahan Thanks for reviewing! I updated some of the formatting based on your feedback. It looks like I've encountered a windows pathing issue in some SDK lib. I'll investigate tomorrow :( | 
| 
 I think we're back in sync now, I'll keep commenting on changes/comments, and take a final look when we think it's done :) thanks. | 
Glues FES into build_runner, which is our first step towards hot reload + build_runner.
The high level workflow is:
build_runner(hot-reload ready).Major changes:
DdcFrontendServerBuilderto our set of DDC builders (enabled via theweb-hot-reloadconfig). This builder keeps aPersistentFrontendServerinstance alive across rebuilds. Compile/recompile requests are queued via aFrontendServerProxyDriverresource.scratch_spaceto record both 1) the main app entrypoint and 2) updated local files from theentrypoint_markerbuilder and themodule_builderbuilder respectively. These are side effects that break certain stateful 'guarantees' of standard build_runner execution. Theentrypoint_markerbuilder runs before any of the downstream DDC builders and finds the web entrypoint, as Frontend Server must receive the same entrypoint on every compilation request.build_runnerbe disabled.Test changes:
build_testto permit incremental builds. This involves passing the asset graph + asset reader/writer across build results and only performing cleanup operations after a series of rebuilds.build_testdoesn't supportruns_beforeand other ordering rules inbuild.yaml, so the above changes allows a kind of imperative ordering, which is important for testingentrypoint_marker.Minor changes:
scratch_spaceso that rebuilds only retain modified files.scratch_spacepackage_config.jsonspecs (packageUriandrootUri). The previous values didn't seem to make sense to me, but I'm also not familiar with how that's standardized inscratch_space.fileanduuiddeps tobuild_modules.Currently doesn't support live-reloading (functionality appears to have been broken a while ago). This'll be added in an upcoming change and permit
webdev-like auto-hot-reload on save (on top of manual).Enable this by adding the following to a project's
build.yaml: