[web] Unify JS configuration. Make it available from initEngine.

[ditman]

Changes

This PR does most of the configuration work (including a stretch goal) described in this document.

In particular, it extends the currently existing plumbing to pass and store Flutter engine configuration from JS (through the initializeEngine method).

The configuration system now behaves like this:

1. The already existing JsFlutterConfiguration JS-interop class is reused in the flutter.js EngineInitializer object so config passing from HTML -> Flutter stays the same across: the global variable, and initEngine call.
2. Users are warned that the old JS global variables (window.flutterConfiguration and window.flutterWebRenderer) are now deprecated, and that they should use engineInitializer.initializeEngine(config) instead.
3. An assertion is thrown in apps that use BOTH configuration methods, so users remember to clean-up the globals in their index.html (in release, it uses whatever comes from the initializeEngine call).

Demo

This PR, not only adds a hostElement parameter (unused for now), but also all the properties defined in the configuration.dart file. For now:

• renderer
• canvasKitBaseUrl
• canvasKitMaximumSurfaces
• canvasKitForceCpuOnly
• hostElement
• debugShowSemanticsNodes

Here's a demo of an app running this code, and configuring everything from its index.html.

Note that the demo has a warning, because it's using a deprecated configuration style:

The demo does fail with an assertion in development mode:

TL;DR API:

//...
onEntrypointLoaded: async function(engineInitializer) {
  let appRunner = await engineInitializer.initializeEngine({
    renderer: 'canvaskit',
    canvasKitBaseUrl: 'canvaskit/',
    canvasKitMaximumSurfaces: 64,
    targetElement: document.querySelector('#flutter_target'),
     // ... other config ...
  });
  appRunner.runApp();
}

Issues

• First step of Render app inside custom html element flutter#32329
• Fixes [web] Allow passing a hostElement via JS config to the engine. flutter#114700
• Fixes [web] Unify configuration into a single entrypoint. flutter#114701

Testing

• Manually deployed: https://dit-tests.web.app
• Added some unit tests:
  • configuration object new features
  • initialization process accepts and stores configuration

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[eyebrowsoffire]

Looks good, with a small nitpick.

One suggestion, take it or leave it: It might be nice if FlutterConfiguration and JSFlutterConfiguration shared the same base class with the different configuration getters on it. That way, if a new field is added to the configuration, the analyzer will tell you that you need to add it to both classes. I'm not sure if this gets weird with the static interop stuff, so if it's too much of a pain feel free to ignore.

[yjbanov]

nit: end dartdocs with a period.

[ditman]

Please, do +1 here: https://github.com/dart-lang/linter/issues/1433 :P

[ditman]

It might be nice if FlutterConfiguration and JSFlutterConfiguration shared the same base class with the different configuration getters on it. That way, if a new field is added to the configuration, the analyzer will tell you that you need to add it to both classes. I'm not sure if this gets weird with the static interop stuff, so if it's too much of a pain feel free to ignore.

@eyebrowsoffire I think that extending js-interop classes with dart classes (and vice versa?) is a big no-no. (Maybe that's why the FlutterConfiguration currently just wraps the JsFlutterConfiguration?)

This is a great suggestion, though! (I'd try it if I didn't have the suspicion that it can break wasm interop :P) One drawback: wouldn't that mean that people need to specify the getter name in 3 places, rather than 2? Also, if they forget to set it in the base class, but not in the JS/Dart implementations, everything would Still Work™.

If I could define the getters somehow ONCE, I'd be 100% sold.

[ditman]

I think I addressed all the comments in the PR. PTAL @yjbanov, @eyebrowsoffire! And thanks for the review!

[ditman]

Please, do make sure we like the names for the new JsFlutterConfiguration values that this PR is adding:

  external DomElement? get hostElement;
  external String? get renderer;

once this lands, they become public API (the other 4 values already existed in the object).

[mdebbar]

[yjbanov]

I'd remove the "before" parts, let's boldly step into the new world! :)

[ditman]

I disagree, I'll remove these docs when we clean up the deprecated code. What I'm going to do, however, is move the new style to the top (and not call it "after"/"new" or anything), and move the legacy style underneath.

[yjbanov]

It might be nice if FlutterConfiguration and JSFlutterConfiguration shared the same base class

@eyebrowsoffire @ditman I've always been suspicious about using inheritance for classes used to pass data, as opposed to delivering behavior. There are some expectations that come with inheritance that may not be appropriate. For example, there's the Liskov substitution principle which doesn't make sense for this case. I've always preferred composition for data over inheritance.

[ditman]

Rebased to see if it heals the failing "impeller" tests.