Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[web] Migrate framework away from dart:html and package:js #128580

Merged
merged 5 commits into from Jun 14, 2023
Merged

[web] Migrate framework away from dart:html and package:js #128580

merged 5 commits into from Jun 14, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Jun 9, 2023 •
edited

Use package:web and dart:js_interop instead.

Part of #127030

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Jun 9, 2023
srujzs
srujzs approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@srujzs srujzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/flutter/test/painting/_test_http_request.dart Outdated Show resolved Hide resolved
packages/flutter/test/widgets/selectable_region_context_menu_test.dart Show resolved Hide resolved
@mdebbar
Copy link
Contributor Author

mdebbar commented Jun 9, 2023

IIUC, this comment:

flutter/dev/bots/allowlist.dart

Lines 13 to 14 in a08a211

/// Before adding a new Dart Team owned dependency to this set, please clear with natebosch@
/// or jakemac53@. For other packages please contact hixie@ or zanderso@.

is asking for @natebosch or @jakemac53 approval for adding the new package:web dependency. Would you mind taking a look?

@jakemac53
Copy link
Contributor

is asking for @natebosch or @jakemac53 approval for adding the new package:web dependency. Would you mind taking a look?

As long as the owners of the package are fine with this and understand the implications, LGTM.

eyebrowsoffire
eyebrowsoffire approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@srujzs
Copy link
Contributor

srujzs commented Jun 9, 2023

@mdebbar Can we potentially hold off on landing this for a few days? We're thinking about changing package:web types to use Dart primitives for ergonomics which may break this code, but I want to have a chat first to confirm before the framework depends on package:web.

@mdebbar
Copy link
Contributor Author

mdebbar commented Jun 9, 2023

Of course!

Converting between dart and JS is an inconvenience for sure. Do you think there's a way to improve this without sacrificing wasm performance?

@srujzs
Copy link
Contributor

srujzs commented Jun 9, 2023

Thanks! For numbers and booleans, we think copy + conversion semantics should be performant enough on dart2wasm. For Strings, @joshualitt is exploring making the underlying implementation of JSString proxy String. We'll still probably need to do the copy if you pass a Dart String, but I think we can avoid the copy cost if you pass a JSString. Of course, we can internalize any constant Strings to make that faster. This new approach is a bit exploratory, so we'll likely need to do some benchmarking.

We're still up in the air about List and JSArray, but I believe the goal is to require the rest of the types to be JS types still.

@srujzs
Copy link
Contributor

srujzs commented Jun 12, 2023

@mdebbar Nevermind, we're going to version-roll package:web, so we shouldn't break you. Feel free to land whenever you're ready, sorry for the noise. :)

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2023
@auto-submit auto-submit bot merged commit 95be76a into flutter:master Jun 14, 2023
139 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 14, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2023
@engine-flutter-autoroll
95be76ab7 [web] Migrate framework away from dart:html and package:js (f…
b62babf 
…lutter/flutter#128580)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 14, 2023
@engine-flutter-autoroll
Manual roll Flutter from 09b7e5615761 to 95be76ab7e3d (14 revisions) (#…
c34ca5d 
…4214)

Manual roll requested by tarrinneal@google.com

flutter/flutter@09b7e56...95be76a

2023-06-14 mdebbar@google.com [web] Migrate framework away from dart:html and package:js (flutter/flutter#128580)
2023-06-14 77465135+cruiser-baxter@users.noreply.github.com Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged (flutter/flutter#128137)
2023-06-14 goderbauer@google.com Inline AbstractNode into SemanticsNode and Layer (flutter/flutter#128467)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 727b4413fe6f to 2d8d5ecfe4a8 (5 revisions) (flutter/flutter#128842)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 66a5761412f9 to 727b4413fe6f (10 revisions) (flutter/flutter#128841)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from b6bf3a6f1ccd to 66a5761412f9 (1 revision) (flutter/flutter#128813)
2023-06-13 william.oprandi+github@gmail.com Fix syntax error in docstring (flutter/flutter#128692)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update unit tests in material library for Material 3 (flutter/flutter#128725)
2023-06-13 christopherfujino@gmail.com [flutter_tools] Suppress git output in flutter channel (flutter/flutter#128475)
2023-06-13 katelovett@google.com Fix ensureVisible and default focus traversal for reversed scrollables (flutter/flutter#128756)
2023-06-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9a0a0dafeea to b6bf3a6f1ccd (2 revisions) (flutter/flutter#128797)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update rest of the unit tests in material library for Material 3 (flutter/flutter#128747)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update tests in material library for Material 3 by default (flutter/flutter#128300)
2023-06-13 hans.muller@gmail.com Update misc tests for Material3 (flutter/flutter#128712)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
@engine-flutter-autoroll
95be76ab7 [web] Migrate framework away from dart:html and package:js (f…
6439254 
…lutter/flutter#128580)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
95be76ab7 [web] Migrate framework away from dart:html and package:js (f…
4f80734 
…lutter/flutter#128580)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
95be76ab7 [web] Migrate framework away from dart:html and package:js (f…
b401e24 
…lutter/flutter#128580)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
95be76ab7 [web] Migrate framework away from dart:html and package:js (f…
c770830 
…lutter/flutter#128580)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srujzs srujzs srujzs approved these changes

@eyebrowsoffire eyebrowsoffire eyebrowsoffire approved these changes

@joshualitt joshualitt Awaiting requested review from joshualitt

Assignees
No one assigned
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mdebbar @jakemac53 @srujzs @eyebrowsoffire