Make it cheaper to flip between DDC and Dart2Js

[natebosch]

Currently it's easy to go from --release back to dev and if nothing has changed very little work needs to be done, the DDC entrypoint builder is cheap and no actual compilation happens.

Going from dev back to --release, however, is slow. It requires re-running dart2js on all entrypoints because the work was overwritten by the ddc bootstrapping.

We could make it cheap to flip back and forth if we write dart2js output to a different file.

• Add an optional builder which mirrors the entrypoint builder for dart2js. Output to .dart.dart2js instead of .dart.js
• Change the entrypoint builder to do a direct copy from .dart.dart2js to .dart.js.

The tricky part here is handling dart2js_args. We probably will need this to be a breaking change and have users move args from build_web_compilers|entrypoint to build_web_compilers|dart2js.

[jakemac53]

The tricky part here is handling dart2js_args. We probably will need this to be a breaking change and have users move args from build_web_compilers|entrypoint to build_web_compilers|dart2js.

We could just make it two builders wrapped up under the one though (add a builder factory). Then the options would get plumbed through correctly as is.

The new builder would be an optional builder so it only runs if the actual entrypoint builder asks for it.

[jakemac53]

Note also though that the use case for this is pretty narrow - it is only when switching from release to debug and back to release without making any code changes that you would get benefit, yet it comes with an extra cost in terms of actions and copying for all release builds.

[natebosch]

We could just make it two builders wrapped up under the one though (add a builder factory). Then the options would get plumbed through correctly as is.

👍

the use case for this is pretty narrow

True. I think the cost in extra actions is probably fairly low and this does have a benefit when iterating on one or a small number of test files and checking behavior with both Dart2Js and DDC.

[jakemac53]

True. I think the cost in extra actions is probably fairly low and this does have a benefit when iterating on one or a small number of test files and checking behavior with both Dart2Js and DDC.

But if you are iterating on them then dart2js is going to re-run anyways ;)

[natebosch]

But if you are iterating on them then dart2js is going to re-run anyways

Not for the tests you haven't touched, but that's only a problem until we're able to solve targeting builds down to the per-test level.

[jakemac53]

Not for the tests you haven't touched

Oh true - if you are iterating on a specific test then this definitely does not give the desired behavior.

but that's only a problem until we're able to solve targeting builds down to the per-test level.

Ya, I am still not completely sure how we want to solve this though. We can build the entrypoint and all the outputs of it, but I don't know how we can track what other potential files may need to be built (css, etc).

[jakemac53]

So one potential solution I guess here would be to update the bootstrapping builder to also look at the test html file, and try and establish a dependency via canRead on all the assets it notices by searching for known attributes (src, href, etc).

This is not foolproof, but would likely work for a lot of cases. Generated non-dart assets that are dynamically injected into the page specifically would not work.

[natebosch]

@jakemac53 - should we do this while we're bumping to 2.0?

[jakemac53]

I think we decided we could do this without a breaking change? I would rather not block on it - we don't see complaints about this that I can remember.

[natebosch]

I took a brief look at this - I think it wouldn't be too hard to do, except we don't support a mix of optional and non-optional builder factories within the same builder definition. Either all of the individual builders are optional, or none are.

I don't know what it would take to allow individual factories in the list to be marked optional, but I do think that would be useful.

@jakemac53 - if we do decide to allow optional builders by index within the list of factories, do you have any ideas how we might want to represent that in the yaml? I could imagine a list with the same length, something like

builders:
  some_name:
    import: "package:foo/builders.dart"
    builder_factories:
      - firstBuilder
      - secondBuilder
    build_extensions:
      .dart:
        - .first_out
        - .second_out
    is_optional:
      - true
      - false

Or maybe allow builder_factories as a map instead?

builders:
  some_name:
    import: "package:foo/builders.dart"
    builder_factories:
      firstBuilder: true
      secondBuilder:
        is_optional: true
    build_extensions:
      .dart:
        - .first_out
        - .second_out

[jakemac53]

Hmmm I don't love either of those options, nor do I have a proposal for a better one. But it does seem generally useful.

[jakemac53]

Is it possible to omit the value entirely for certain entries (in the map)?

[natebosch]

Yes, I think we could omit the true placeholder - when we parse it we'll get null.

builders:
 some_name:
   import: "package:foo/builders.dart"
   builder_factories:
     firstBuilder:
     secondBuilder:
       is_optional: true
   build_extensions:
     .dart:
       - .first_out
       - .second_out

[jakemac53]

I don't think that looks too bad. It would just presumably be an override of the general setting? Is there any other setting that we should expose (source vs cache)?

[natebosch]

Is there any other setting that we should expose (source vs cache)?

There likely is, but I don't think we should add generality without a specific use case in mind. I think we can add additional config options in a non-breaking way as we need them.

Adding support for this option will require a breaking change for https://pub.dev/documentation/build_runner_core/latest/build_runner_core/apply.html though, so maybe we should decide ahead of time what other options to support, and only break that interface once. We could hold back support in build_config safely until we have a use case, or plumb it through eagerly anyway.

It would just presumably be an override of the general setting?

Yeah I think so. Could consider making it an error to specify for both, but I don't see any reason to not allow something like

   builder_factories:
     firstBuilder:
       is_optional: false
     secondBuilder:
     thirdBuilder:
   is_optional: true

[jakemac53]

That all sounds reasonable to me, I would probably go ahead and support overriding the source vs cache configuration too since I think it would be fairly trivial and I could definitely imagine use cases.

Fwiw the reason I closed this originally though is that to my knowledge no users have asked for it. They may not be aware they should want it (or could get improvement in this area), but in general it seemed like not a necessary thing to have.

But if you want to take some time to fix it, I would be happy to do reviews etc. And I agree this is a generally sensible feature to have (overriding is_optional, built_to, per factory).