Skip to content

[rfw] Support web (as JS) #4650

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
Merged
merged 1 commit into from
Aug 23, 2023
Merged

[rfw] Support web (as JS) #4650

merged 1 commit into from
Aug 23, 2023

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Aug 4, 2023

Pre-launch Checklist

Fixes flutter/flutter#129843

  • 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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@github-actions github-actions bot added the p: rfw Remote Flutter Widgets label Aug 4, 2023
@Hixie Hixie force-pushed the rfwweb branch 4 times, most recently from f2c41dc to af60c5e Compare August 7, 2023 18:57
@Hixie Hixie requested a review from stuartmorgan-g August 14, 2023 21:16
stuartmorgan-g
stuartmorgan-g approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM with some optional nits.

packages/rfw/CHANGELOG.md Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Improves

(Per style guide linked from the checklist.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

packages/rfw/lib/src/dart/binary.dart Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Effective Dart says not to use k, and Flutter style only says to use it for global constants

(I would strongly vote for changing the Flutter style guide to remove the _k... example and change "It’s not necessary to add the k prefix to non-global constants." to explicitly forbid it, to reduce pointless diff from Effective Dart.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

(go for it!)

packages/rfw/test/utils.dart Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the Flutter channel renaming actually happening soon? If not, this seems like confusing naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

master and main are synonyms now for flutter/flutter.

@Hixie
Copy link
Contributor Author

Hixie commented Aug 17, 2023

updated per comment, will autoland on green. Feel free to remove the autosubmit label if you want additional changes (e.g. if you want the main/master thing changed).

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 17, 2023

auto label is removed for flutter/packages/4650, due to Pull request flutter/packages/4650 is not in a mergeable state.

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 23, 2023
@auto-submit auto-submit bot merged commit 3060b1a into flutter:main Aug 23, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 23, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 23, 2023
@engine-flutter-autoroll
3060b1a [rfw] Support web (as JS) (flutter/packages#4650)
6549009
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 23, 2023
@engine-flutter-autoroll
Roll Packages from c730a90 to 3060b1a (4 revisions) (#133149)
bd836cc 
flutter/packages@c730a90...3060b1a

2023-08-23 ian@hixie.ch [rfw] Support web (as JS) (flutter/packages#4650)
2023-08-22 84124091+opxdelwin@users.noreply.github.com [webview_flutter] Update sample code. (flutter/packages#4727)
2023-08-22 ctrysbita@outlook.com [flutter_adaptive_scaffold] Fix top padding for NavigationBar (flutter/packages#4661)
2023-08-22 31859944+LongCatIsLooong@users.noreply.github.com Remove deprecated `ImageProvider` methods (flutter/packages#4725)

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-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@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://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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: rfw Remote Flutter Widgets

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[rfw] Many unit test failures in browser mode

2 participants

@Hixie @stuartmorgan-g