build
build copied to clipboard
dart-lang
Update build_web_compilers:entrypoint outputs to support multiple DDC apps on the same page
Changes
Currently, serving a page that loads more than one .dart.js entrypoint (when they are compiled with DDC) results in only the last one actually executing. This is due to the way the entrypoint JS loads requirejs and queues up the loading of its bootstrap file. Previously, it used the data-main attribute on the require.js <script> element, but that only works if there's only one main to call. In the case of multiple entrypoints, each one effectively overwrites the previous data-main.
This commit changes the way the bootstrap module is loaded and executed to a way that is compatible with multiple DDC entrypoints on the same page. This is accomplished by generating an additional .bootstrap.require.js output from the build_web_compilers|entrypoint builder that will be responsible for calling require([...]) with the bootstrap module name, and then updating the entrypoint JS output to load this new asset after loading require.js.
It also changes the logic responsible for populating window.$dartLoader so that it appends entrypoint-specific data rather than setting it only once. This allows multiple entrypoints to run and augment that global instead of only the first.
Functionally, this accomplishes our goal of being able to at least load and run multiple DDC entrypoints on the same page without any errors. But there may be a better way to do this that maybe doesn't require an extra builder output. I originally tried adding that call to require(["<module_name>"]); via an inline script, but because the require.js script is deferred loaded, the inline script was running too early. I also tried setting up an onload listener for require.js, but that got complicated because each entrypoint adds its own
Open to suggestions on any and all of this!
Testing
For testing purposes, I've been using a very simple app:
<!-- web/index.html -->
<!DOCTYPE html>
<html>
<head>
<title>App with multiple DDC entrypoints</title>
</head>
<body>
<h1>App</h1>
<p id="one">Loading one.dart...</p>
<p id="two">Loading two.dart...</p>
<script defer src="one.dart.js"></script>
<script defer src="two.dart.js"></script>
</body>
</html>
// web/one.dart
import 'dart:async';
import 'dart:html';
void main() {
querySelector('#one').innerText = 'one.dart loaded';
print('one');
}
// web/two.dart
import 'dart:async';
import 'dart:html';
void main() {
querySelector('#two').innerText = 'two.dart loaded';
print('two');
}
Without these changes, you'd see only the second entrypoint run. With the changes, both run.
Note: if serving with webdev, you'll also need --no-injected-client as the webdev server and injected client code make some similar assumptions about only having one entrypoint on the page. I'm going to put up a similar PR to webdev.
I will work on another PR to fix up the recent analyzer deprecations in parallel here to get the build green first.
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.
We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.
Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).
ℹ️ Googlers: Go here for more info.
![google-cla[bot] avatar](https://avatars.githubusercontent.com/in/42202?v=4 "Published by a pub.dev verified publisher"){kind=small}
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.
We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.
Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).
ℹ️ Googlers: Go here for more info.
The more concerning bit is the call to require.config. I thought that was a global thing so I think we might be bashing over the previous app config with the new one. I have a feeling this probably presents some race conditions in app loading.
I had this concern, too, but in stepping through the require.js code it seemed to be safe enough. Each call configures three things:
require.config({
baseUrl: baseUrl,
waitSeconds: 0,
paths: modulePaths
});
Afaict, the baseUrl should be the same for each entrypoint because it is specific to the document and not the scripts. The waitSeconds is hardcoded to 0. And the paths get merged together if config() is called more than once.
I think it is probably worth trying out using the config parameter https://requirejs.org/docs/api.html#multiversion. I have no experience with it but I think it should allow us to do what we want here.
I saw that as well, but didn't spend much time playing with it. I'll see if I can get something working with that approach.
Note also that today all apps on the page share state with each other, see the example I added. I think using the multiversion stuff with require.js might resolve that?
Interesting, didn't realize that! Probably not what we want :)
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.
We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.
Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).
ℹ️ Googlers: Go here for more info.
Actually I just pushed an update here to use a custom context argument - it does give each app its own global state now and appears to work as expected 👍 .
We probably want to clean up a bit exactly what I did there though. We should probably create a namespace for where we put those custom require objects or something.
Interesting, didn't realize that! Probably not what we want :)
Ya, especially since in dart2js they wouldn't share state. I heard people like their development compiler semantics to match up with their release compiler semantics generally :D
Looks like we need to merge master here to pick up the analyzer fixes, I will let you do that though to avoid too many merge conflicts between us haha.
Looks like I have a couple tests to update (and maybe some to add, too). I've got meetings for the next couple of hours but will take a stab at it afterwards.
One other note here, if this breaks webdev serve we should release it as a breaking change. And then webdev will need to update the build_web_compilers constraints it checks for (since its globally activated it does its own checks for the current version of build _web_compilers and complains if it isn't compatible).
If the changes to $dartLoader are breaking, I think we could also probably work around that by introducing new fields instead of changing the existing ones and then webdev could switch to using the new ones and update the minimum constraint on build_web_compilers.
Ya, it would be ideal if we can release it as non-breaking. But breaking changes in this package aren't too painful (other packages don't really depend on it).
I'm not sure what's up with that test failure. Running the same command locally works for me, so maybe it's windows-specific?
Unfortunately this also passes for me locally on windows. Not sure what could be causing it to fail on github?
Actually looking at the error it is failing to load this file: http://localhost:50021/Vma2b75L2B5pZ90MHWi%2F3N6QvT8Wk16f/test/subdir_test.dart.browser_test.dart.bootstrap.js.
That looks to be missing the sub-dir/ directory so it is trying to load the wrong path.
~Probably that means the base url isn't getting set properly?~ nevermind I think I was confused about how that is supposed to work
Ok I figured this out - the bootstrap module isn't actually defined yet by the time this require.js script is running, in github - on windows only. Basically its a race condition in the loading of the scripts. And I guess github windows has a crappy internet connection or loads scripts reliably differently haha.
You can reproduce the error by throttling your network to "Slow 3g" in chrome devtools. That causes the require bootstrap to load first.
Maybe if the bootstrap.js script loaded the require bootstrap script (after it has defined that module), instead of the .dart.js script injecting them both, then it would work out?
If the regular bootstrap script isn't yet known by the time the require bootstrap runs, then it tries to download it by name and fails since the name doesn't match the path.
This does violate my understanding of how defer scripts work... I thought they still ran in source order but that doesn't seem to be the case. I am suprised we don't see similar issues with requirejs itself or the stack trace mapper not being loaded yet, as those are larger scripts.
We could also possibly just remove the defer attribute from all but the require bootstrap script?
Also now I can't get it to reproduce lol... but it did reproduce once for me. 🤕
Ok I pushed another change here that removes the extra bootstrap file, and instead inlines it in the original bootstrap file, after defining the bootstrap module. So it basically just immediately runs it after defining it.
If that works, we should delete the other stuff around that new file (I didn't clean it all up).
@evanweible-wf any concerns around the new approach? It seems to make windows happy.
Nope, this approach looks great to me! I also started seeing this issue this morning until I pulled your latest commits, which fixed it. Although like you, I'm a bit confused as to why this was an issue.. the MDN docs say:
Scripts with the defer attribute will execute in the order in which they appear in the document.
¯_(ツ)_/¯ regardless, this approach is simpler and seems to work 👍
Ok, do you mind pushing a cleanup commit to remove the changes to the build extensions then? And then I can take another pass here :).
Ok, this generally LGTM at this point to me. I think we need to finalize the release strategy still (is this actually breaking for external webdev users or not?).
It would also be nice to get a multi-app test going, but it might be difficult to do. The testing package isn't really set up for that, but we could probably still manage something. I will see if I can come up with something that works here.
@jakemac53 Even if the change to the data on the window like $dartLoader.appDigests isn't breaking, the use of requirejs contexts is as it currently stands. If you try to use webdev's debugger or --auto=restart, you'll hit this error:
Error: Module name "dart_sdk" has not been loaded yet for context: _. Use require([])
I mentioned this over on the webdev PR, but I don't see a good way of working around that without updating webdev first - I think it's going to have to use one or all of the require objects on $dartRequire instead of the default require.
@evanweible-wf should we consider making this feature opt-in behind some builder configuration? That would make it non-breaking, and would also allow us to land it without proper webdev support in place yet.
Yeah that's a good idea! On the webdev side, would we read that same builder config to check for opt-in or just inspect the window/build outputs to determine whether it's being used?
Also, sorry I stepped away from this for a bit - we've been heads down getting to 2.13, but that's almost done now and I'll be able to dedicate more time to the webdev side of things.
Yeah that's a good idea! On the webdev side, would we read that same builder config to check for opt-in or just inspect the window/build outputs to determine whether it's being used?
I don't think webdev could reasonably check the actual config - it could either infer it by looking at the outputs or we could expose something specific on the window that tells it which mode to be in?
Also, sorry I stepped away from this for a bit - we've been heads down getting to 2.13, but that's almost done now and I'll be able to dedicate more time to the webdev side of things.
No worries, just thinking about how we can move this portion forward without figuring out all the webdev stuff :).