Bundle flutter.js via esbuild #47573
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Btw, testing this in presubmit here: flutter/flutter#137113 |
| # flutter.js bundled source and sourcemap | ||
| sources += [ | ||
| "$root_out_dir/flutter_web_sdk/flutter_js/flutter.js", | ||
| "$root_out_dir/flutter_web_sdk/flutter_js/flutter.js.map", |
There was a problem hiding this comment.
We should publish both optimized and raw flutter.js so that it is possible to troubleshoot issues in debug and profile builds. In fact, until the size of flutter.js is more than a blip, I wouldn't bother minifying, just strip the TypeScript type annotations. Alternatively, if we could roll esbuild all the way to the Flutter SDK (how big is the binary?), and then provide optimization/minification options in the Flutter Tool. This way they engine build of flutter.js can be distributed in original source.
There was a problem hiding this comment.
We actually are publishing both with this PR now! We ship the minified version, the original source, and the sourcemaps that map between them.
There was a problem hiding this comment.
Good question! about how big it is without minification?
There was a problem hiding this comment.
Right now uncompressed it's 14kb before minification, 4kb after, so not terribly significant. The main reason to be bundling is to merge multiple files into one so that we don't have a complex waterfall with multiple roundtrips. Obviously right now there is just a single file, but I'm trying to prepare to modularize it into separate .ts files. Since we have sourcemaps and are shipping the original source, I don't really see a downside to actually minifying it since we're already bundling though.
|
|
||
| esbuild("flutter_js_bundle") { | ||
| entry_point = "$root_out_dir/flutter_web_sdk/flutter_js/src/flutter.js" | ||
| output_bundle = "$root_out_dir/flutter_web_sdk/flutter_js" |
There was a problem hiding this comment.
do we want this to be $root_out_dir/flutter_web_sdk/flutter_js/dist or similar? (this might be problematic for the sourcemaps maybe?)
There was a problem hiding this comment.
Yeah that will end up with the sourcemaps mapping to ../src/flutter.js, which I don't think is going to work well when served via http. I'm not exactly sure how to around that issue with a dist folder to be honest.
build/esbuild/esbuild.gni
Outdated
| "--bundle", | ||
| "--minify", | ||
| "--sourcemap", |
There was a problem hiding this comment.
Can these 3 args be passed in from the invoker? That way the esbuild action is more generic, and the operations that are performed on the source are defined close to where the actual flutter.js build target is defined?
| esbuild("flutter_js_bundle") { | ||
| entry_point = "$root_out_dir/flutter_web_sdk/flutter_js/src/flutter.js" | ||
| output_bundle = "$root_out_dir/flutter_web_sdk/flutter_js" | ||
|
|
eyebrowsoffire
added
the
autosubmit
…137872) flutter/engine@035740c...ec20731 2023-11-03 skia-flutter-autoroll@skia.org Roll Dart SDK from 529fcd5bdffc to c52fb4fe2d86 (1 revision) (flutter/engine#47668) 2023-11-03 jacksongardner@google.com Bundle flutter.js via esbuild (flutter/engine#47573) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
PR #47573 renamed the version-lock file from `browser_lock.yaml` to `package_lock.yaml`. This PR adjusts sources to changes. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
browser_lock/browser_rolleretc topackage_lockandpackage_rollersince it will handle more than just browsers now.flutter.jsusing esbuild.flutter_web_sdkthe originalflutter.jssource, the minifiedflutter.js, and a sourcemap file to map between thnm.flutter.jsstuff influtter_web_sdk/flutter_jsinstead of just at the root level offlutter_web_sdk. This should be fine because I haven't merged the change that has the flutter tool consume this yet.