Extract playground components #23253

alexeyinkin · 2022-09-15T08:36:28Z

In order to embed Playground into Tour of Beam, reusable code must be extracted from Playground into a shared package.

This PR:

Extracts a lot of code into playground_components package.
Adds easy_localization to Playground to localize strings related to the shared package. In the future, the entire Playground will be migrated to easy_localization for consistency.
Converts enum SDK to class Sdk so that the frontend may receive examples in a language it is not aware of. This is not a complete feature yet. In the future, we may allow such code to be shown but not be runnable. This is useful because in Beam docs there are now occasional TypeScript snippets for which we do not have a runner (although they are on the website only and not in the DB). With a closed enum, we would not be able to show such snippets at all becasue the embedded playground cannot be combined with ordinary HTML snippets under a signle set of tabs.
Renames PlaygroundState to PlaygroundController. Traditionally such change notifiers are called controllers in Flutter, e.g. TextEditingController, TabController, ScrollController, etc.
Renames ExamplesState to ExampleCache to limit its responsibility which is not defined now. In addition to caching catalog examples, it still performs non-cached calls like getting an example by path (for which we can introduce caching later), but it has functionality even more alien than that which will be removed soon.
Introduces a theme extension to store the app's colors and styles which is new to Flutter 3. Drops separate classes and inherited widgets that were used for this before.
Introduces ExampleBase and Example instead of a single ExampleModel class. Before there was no distinction between a codeless example pre-fetched in a list and a fully fetched example, and example's code had to be inspected to tell one from another. The two new classes allow for compile-time check that example code and other full info is fetched. The new classes are also immutable for even more safety.
Has some other renamings that improve clarity.
Adds the tour screen into Tour of Beam app with an embedded Playground.
Removes all generated mocks from version control. They are now generated at deployment and must be manually generated for local runs.
Adds playground:frontend:playground_components Gradle projects with precommit as the most notable task. Updates playgound:frontend:precommit and friends to account for the new package.

This PR focuses on establishing a more or less stable interface between the two apps and the shared package. It blocks all work on the entire frontend due to its scale, so with any local issues we still prefer this merged to work on them later separately.

I carefully did git mv when moving files, but it turned out that it is insufficient for git to recognize renaming (it happens to only compare file contents anyway), and many moved files appear deleted and added. If it is a problem for the review, please tell me so, and I will make a proper branch, although it might take a day or so.

I guess the only guaranteed way is to:

Move a file.
Commit.
Edit the file.
Commit again.

Next time I will do it the right way.

This work aggregates the Tour of Beam screen and the Playground refactoring in a single PR because this was the fastest workflow that allowed us to avoid waiting for merges. Thank @nausharipov for the ToB part.

Resolves #22600

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

* theme setup * Replaced ThemeProvider with ThemeSwitchNotifier * header with theme mode switcher and logo * page container with header & footer * theme mode tests * renamed the directory to tour-of-beam * compressed beam_logo.png * added missing license comments * rudimentary layout of the first screen * review comments fixes #1 * moved notifyListeners inside then * responsive todo * split into 2 simple functions * deleted redundant constants & replaced 2018 text theme with 2021 * styling refinement * home screen layout * clickable sign in text * font weights fix * removed _getBaseFontTheme function * fixed border and bg color * color fixes * difficulty component * _LastModuleBody * todo in test * footer border * fixed overflows * replaced Project prefix with Tob * replaced then with await * inferred type * started translation of the home screen * sorted translations * Complexity comments * comment fixes * home screen translations * sign in overlay * import fix * integration test does not fail * playground_components package with dismissible_overlay * missing license * removed _dots from build * widgets refinement * renamed home screen to welcome screen * deleted copyWith * _SdkButton * trailing comma & pubspec formatting * license and lints * license * removed license from .metadata * pubspec formatting * total lints update * changed from tour_of_beam to tour-of-beam in build.gradle.kts * license check * _SdkButton mimics Radio button * renamed MyApp to TourOfBeamApp * onChanged mimics Radio button Tour of Beam frontend blank project [Tour of Beam][Frontend][apache#22600] TourScreen layout TourScreen layout (apache#22600) common theme, constants, split view missing license flutter_gen, summary layout details content layout details no functional widgets in split view main screen todos & translation main screen todos & translation comment fixes #1 ExpansionTileWrapper SplitViewController lists in tour screen widgets comment fixes #1 (31.08) split view package in PGC fixed button overflow splitter theme color comment fixes #2 (31.08) gradlew check welcome screen overflow test (apache#22600) SDK dropdown (apache#22600) flexible complete unit OutlinedButton (apache#22600) renamed PageContainer to TobScaffold dropdown style refinement DropdownButton implicit type sdk instead of e licenses apache#22600 renamed _ShrinkedTour to _NarrowTour apache#22600 tour screen style refinement apache#22600 BeamDivider in PGC apache#22600 removed todo, added license apache#22600 built with text apache#22600 _WideWelcome with IntrinsicHeight (apache#22600) Co-Authored-By: darkhan.nausharipov <darkhan.nausharipov@kzn.akvelon.com>

replaced magic numbers apache#22600 comments (apache#22600) comments apache#22600 comments apache#22600 comments apache#22600 comments apache#22600 comments, flutter 3.3.0 upgrade apache#22600 renamed ActionPadding to ActionVerticalPadding apache#22600 actions formatting apache#22600

alexeyinkin · 2022-09-15T11:33:51Z

playground/frontend/lib/l10n/app_en.arb

-  "reset": "Reset",
-  "@reset": {
-    "description": "Title for the reset button"
-  },


Moving to easy_localization's YAML files.

alexeyinkin · 2022-09-15T12:12:10Z

playground/frontend/lib/pages/embedded_playground/components/embedded_actions.dart

    // The empty list forces the parsing of EmptyExampleLoadingDescriptor
    // and prevents the glimpse of the default catalog example.
    final window = html.window.open(
-      '/?$kExamplesParam=[]&$kSdkParam=${state.sdk?.name}',


https://medium.com/flutter-senior/never-use-name-as-a-variable-4a18e46fa85c

alexeyinkin · 2022-09-15T12:17:08Z

playground/frontend/lib/pages/embedded_playground/components/embedded_editor.dart

-    if (controller == null) {
-      return const LoadingIndicator(size: kLgLoadingIndicatorSize);
+    if (snippetController == null) {
+      return const LoadingIndicator();


We had two sizes of the indicator: 40px and 50px.
I don't think it was a formal requirement for the two sizes. Can we go with just one of them simplifying things for Apache to maintain after we are done? (Given that I added them instead of an empty editor in the first place.)

alexeyinkin · 2022-09-15T12:27:31Z

playground/frontend/playground_components/lib/src/api/iis_workaround_channel_non_web.dart

+
+import 'package:grpc/grpc_connection_interface.dart';
+
+class IisWorkaroundChannel extends ClientChannelBase {


This non-web alternative is required to run tests as native apps which is faster. Without it, such tests do not build citing 'dart:html not available'.

alexeyinkin · 2022-09-15T12:28:40Z

playground/frontend/playground_components/lib/src/cache/example_cache.dart

+import '../repositories/models/get_snippet_request.dart';
+import '../repositories/models/save_snippet_request.dart';
+
+class ExampleCache extends ChangeNotifier {


This was ExampleState, git did not recognize the moving.

alexeyinkin · 2022-09-15T12:42:41Z

...ground/frontend/playground_components/lib/src/repositories/code_client/grpc_code_client.dart

+
+const kGeneralError = 'Failed to execute code';
+
+class GrpcCodeClient implements CodeClient {


Moved, git did not recognize that.

alexeyinkin · 2022-09-15T12:49:02Z

playground/frontend/pubspec.yaml

  provider: ^6.0.0
  shared_preferences: ^2.0.12
  url_launcher: ^6.0.12
+  url_strategy: ^0.2.0


One less manual check for dart:html.

nausharipov · 2022-09-16T05:41:50Z

learning/tour-of-beam/frontend/assets/translations/en.yaml

+  signIn: Sign in
+  signOut: Sign out
+  toWebsite: To Apache Beam website
+  deleteAccount: Delete my account


Not in alphabetic order.

nausharipov · 2022-09-16T05:49:44Z

learning/tour-of-beam/frontend/lib/components/login/login_content.dart

+        horizontal: BeamSizes.size24,
+      ),
+    );
+    const minimumSize = MaterialStatePropertyAll(Size(double.infinity, 0));


Is it better to use BeamSizes for double.infinity and 0?

In this file there are more things that will need to be reused, like button styles themselves. Let's wait for this need to see how to do this better.

nausharipov · 2022-09-16T05:51:23Z

learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart

+import 'package:flutter/widgets.dart';
+import 'package:playground_components/playground_components.dart';
+
+// This is for demo only. Need a thought-though import in production.


A typo: thought-though.

nausharipov · 2022-09-16T05:54:34Z

learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart

+}
+
+class _PlaygroundDemoWidgetState extends State<PlaygroundDemoWidget> {
+  late final PlaygroundController playgroundController;


Is it possible to get rid of the late?

My favorite way was to create dependencies in createState and then pass the result objects in the state constructor. But then I discovered that is discouraged by https://dart-lang.github.io/linter/lints/no_logic_in_create_state.html

Anyway this is only to show the window at the meeting.

In production, there will be a separate
TourScreenNotifier extends ChangeNotifier with PageStateNotifier<void>

Nothing stops us from creating PlaygroundController with all its dependencies in its constructor (if it is a factory constructor).

nausharipov · 2022-09-16T05:58:46Z

learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart

+      examplesLoader: ExamplesLoader(),
+    );
+
+    playgroundController.examplesLoader.load(


Is it correct to do all of this in initState?

Should it be extracted into a function?

nausharipov · 2022-09-16T06:03:39Z

learning/tour-of-beam/frontend/lib/pages/tour/screen.dart

+  @override
+  Widget build(BuildContext context) {
+    return Container(
+      width: 250,


Should be extracted as _contentTreeWidth constant.

I will let you continue with this file once it is merged.

nausharipov · 2022-09-16T06:14:53Z

playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs_headers.dart

+    return Consumer<PlaygroundController>(
+      builder: (context, controller, child) {
+        return SizedBox(
+          width: _width,


The name could be more specific: _tabBarWidth.

This was only indented but not added. I would rather not add more refactoring.

This PR focuses on establishing a more or less stable interface between the two apps and the shared package. All the internals of the three will be addressed later.

Malarg · 2022-09-16T07:40:54Z

learning/tour-of-beam/frontend/lib/components/login/login_content.dart

+  }
+}
+
+class _Body extends StatelessWidget {


Why this widget exists? It is not reused anywhere, so we can just put this login into LofinContext.build method. Or if it exist for extracting some login, I think it should has more accurate name. _Background for example

I extract widgets to also improve readability.

Having this widget in the build method would decrease readability.

I associate _Background with something behind a widget and with a color.

I am on the fence. Its client's build consists of only creating this widget, so it feels redundant. On the other hand it is too minor issue to fix.
Feel free to go either way if you are to edit this file later.

Malarg · 2022-09-16T07:53:40Z

learning/tour-of-beam/frontend/lib/components/scaffold.dart

+import 'profile/avatar.dart';
+import 'sdk_dropdown.dart';
+
+class TobScaffold extends StatelessWidget {


I have a habit to name files the same way, as class name inside it. This is not required, but I consider it as a good practice. Have you heard of this approach? Why don't you use it?

This is common when a file per class is enforced.
Dart has no prescription for this because global code like variables, constants, typedefs, etc. are commonly used.
Instead Dart enforces lowercase file names which already breaks the strictness of file naming and some of its benefits (like automated file location, searching for uses and file inclusions in a single find-in-files search, etc.).

On top of that, I see the following reasons to stray from name matches:

I like naming widgets with Widget at the end so we don't trap ourselves like Flutter did reserving Image class for a widget. I only do otherwise if the widget suffix makes it obvious like Button. This would make most of the widget files end with _widget.dart which clutters the view.

Renaming identifiers is a good practice when we find more precise names for identifiers. This is easy if only the code but not the file names. Renaming files requires caution as we have seen in this PR which confused git a lot. So file renaming should be avoided. This mismatch is a factor to untie the two.

But I am open to tie. @nausharipov what do you think?

@alexeyinkin your 2 points make total sense to me.

Malarg · 2022-09-16T07:56:56Z

learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart

+    'https://backend-scio-beta-dot-apache-beam-testing.appspot.com';
+
+class PlaygroundDemoWidget extends StatefulWidget {
+  const PlaygroundDemoWidget({Key? key}) : super(key: key);


Now you can write just const PlaygroundDemoWidget({super.key}); :)

That's my IDE's template. I change this for code that lasts. This is a short-lived file for POC. :)

Malarg · 2022-09-16T07:57:29Z

learning/tour-of-beam/frontend/lib/pages/tour/playground_demo.dart

+    );
+
+    playgroundController.examplesLoader.load(
+      ExamplesLoadingDescriptor(


can be const

…ion to frontend Gradle task, remove generated mocks, fix linter issues (apache#22600)

alexeyinkin · 2022-09-16T14:25:26Z

.gitignore

@@ -125,6 +125,7 @@ website/www/yarn-error.log
 **/.flutter-plugins
 **/.flutter-plugins-dependencies
 **/generated_plugin_registrant.dart
+**/*.mocks.dart


We now generate the mocks after checkout. See the Dockerfile update.

alexeyinkin · 2022-09-16T14:29:29Z

playground/frontend/build.gradle

-  doLast {
-    exec {
-      executable("rm")
-      args("-r", "build")


flutter clean is added instead which additionally deletes .dart_tool directory. Somehow this is required for flutter_gen_runner package which itself looks more like a bug, but still.

alexeyinkin · 2022-09-16T14:32:50Z

playground/frontend/build.gradle

+  }
+}
+
+task flutterClean {


codecov · 2022-09-16T14:41:50Z

Codecov Report

Merging #23253 (e00889a) into master (c78fc9a) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #23253      +/-   ##
==========================================
- Coverage   73.60%   73.44%   -0.17%     
==========================================
  Files         716      718       +2     
  Lines       95289    95528     +239     
==========================================
+ Hits        70141    70156      +15     
- Misses      23852    24076     +224     
  Partials     1296     1296

Flag	Coverage Δ
python	`83.16% <ø> (-0.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../python/apache_beam/testing/test_stream_service.py	`88.09% <0.00%> (-4.77%)`	⬇️
...python/apache_beam/runners/worker/worker_status.py	`74.66% <0.00%> (-2.00%)`	⬇️
...ks/python/apache_beam/runners/worker/data_plane.py	`87.57% <0.00%> (-1.70%)`	⬇️
...che_beam/runners/interactive/interactive_runner.py	`90.06% <0.00%> (-1.33%)`	⬇️
...hon/apache_beam/runners/direct/test_stream_impl.py	`93.28% <0.00%> (-0.75%)`	⬇️
...eam/runners/portability/fn_api_runner/execution.py	`92.44% <0.00%> (-0.65%)`	⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py	`88.62% <0.00%> (-0.48%)`	⬇️
...s/python/apache_beam/io/gcp/bigquery_file_loads.py	`87.24% <0.00%> (-0.47%)`	⬇️
...beam/runners/portability/local_job_service_main.py	`14.43% <0.00%> (-0.42%)`	⬇️
...hon/apache_beam/runners/worker/bundle_processor.py	`93.30% <0.00%> (-0.38%)`	⬇️
... and 15 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alexeyinkin · 2022-09-16T14:48:17Z

R: @damondouglas

github-actions · 2022-09-16T15:19:07Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damccorm for label build.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

github-actions · 2022-09-16T15:20:41Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

damondouglas

@pabloem LGTM
This is incredible work. @alexeyinkin some requests maybe for a future PR.

I had to follow https://pub.dev/packages/flutter_gen instructions within the playground_components directory to remove the build/run errors associated with missing generated files. Perhaps we should add an additional instruction in the README for someone to perform flutter packages pub run build_runner build as part of local execution/build.
I notice that both the Share my code and Run buttons are missing the pointer cursor on hover. It appears as an edit cursor on hover. This is on a Chrome browser from my Linux machine, Chromebook as well as MacOs. The currently deployed play.beam.apache.org has the same issue for the current Run button.

pabloem · 2022-09-24T19:45:24Z

lgtm!

alexeyinkin · 2022-09-26T05:35:19Z

It's already there: https://github.com/apache/beam/tree/master/playground/frontend#getting-started
The next PR will have a single Gradle task to set up everything for running.
Thanks, filed: [Bug]: Editor mouse cursor instead of pointer cursor over the run button #23367

ahmedabu98 · 2022-10-05T21:32:53Z

This PR broke beam_Release_NightlySnapshot: https://ci-beam.apache.org/job/beam_Release_NightlySnapshot/1680/

alexeyinkin and others added 11 commits September 5, 2022 17:14

branded sign in buttons apache#22600

8222fae

_BrandedSignInButtons apache#22600

dd585bc

_Divider color apache#22600

f9ed0ce

profile apache#22600

999e916

moved split_view from PGC into ToB apache#22600

1d1ff90

indentation fix apache#22600

dfca997

split ProfileContent into widgets apache#22600

f8e462d

Merge branch 'master' into 22600_tob_main_page

ad2d51d

Extract playground components to a separate package (apache#22600)

0ac6513

github-actions bot added the learning label Sep 15, 2022

alexeyinkin commented Sep 15, 2022

View reviewed changes

Minor fixes (apache#22600)

d735c71

nausharipov reviewed Sep 16, 2022

View reviewed changes

Malarg reviewed Sep 16, 2022

View reviewed changes

alexeyinkin added 2 commits September 16, 2022 12:30

Address review issues (apache#22600)

1baa787

Upgrade Flutter to v3.3.2 (apache#22600)

f6dc5bc

github-actions bot added the build label Sep 16, 2022

Add precommit Gradle task for playground_components, add code generat…

09c32b6

…ion to frontend Gradle task, remove generated mocks, fix linter issues (apache#22600)

alexeyinkin commented Sep 16, 2022

View reviewed changes

playground/frontend/build.gradle

}

}

task flutterClean {

Copy link

Contributor Author

alexeyinkin Sep 16, 2022

Choose a reason for hiding this comment

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

See #23273

alexeyinkin marked this pull request as ready for review September 16, 2022 14:48

github-actions bot added the Next Action: Reviewers label Sep 16, 2022

darkhan.nausharipov and others added 3 commits September 23, 2022 11:17

startTour button (apache#22600)

14858f8

lint fixes (apache#22600)

63919e4

Fix highlighting for Python and SCIO (apache#22600)

e00889a

damondouglas approved these changes Sep 23, 2022

View reviewed changes

pabloem merged commit c4fe823 into apache:master Sep 24, 2022

alexeyinkin mentioned this pull request Oct 6, 2022

[Bug]: Nightly Snapshot build is broken #23514

Closed

nausharipov deleted the pg_22600_extract-playground-components branch May 15, 2023 14:50


		import 'package:grpc/grpc_connection_interface.dart';

		class IisWorkaroundChannel extends ClientChannelBase {


		const kGeneralError = 'Failed to execute code';

		class GrpcCodeClient implements CodeClient {

Extract playground components #23253

Extract playground components #23253

Conversation

alexeyinkin commented Sep 15, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nausharipov Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nausharipov Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Malarg Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

nausharipov Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

alexeyinkin commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

github-actions bot commented Sep 16, 2022

damondouglas left a comment

Choose a reason for hiding this comment

pabloem commented Sep 24, 2022

alexeyinkin commented Sep 26, 2022

ahmedabu98 commented Oct 5, 2022

alexeyinkin commented Sep 15, 2022 •

edited

Loading

nausharipov Sep 16, 2022 •

edited

Loading

nausharipov Sep 16, 2022 •

edited

Loading

Malarg Sep 16, 2022 •

edited

Loading

nausharipov Sep 16, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading