From 46598122f7e6c77bd904195f82840400a55470d2 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Wed, 17 May 2023 14:27:14 -0400 Subject: [PATCH] [web] Simplify handling of custom url strategy (flutter/engine#42043) Issue: https://github.com/flutter/flutter/issues/126831 --- .../web_ui/lib/src/engine/initialization.dart | 5 +- .../lib/web_ui/lib/src/engine/window.dart | 42 +++---------- .../src/ui_web/navigation/url_strategy.dart | 58 +++++++++++++++++- .../lib/web_ui/test/ui/url_strategy_test.dart | 61 +++++++++++++++++++ 4 files changed, 129 insertions(+), 37 deletions(-) create mode 100644 engine/src/flutter/lib/web_ui/test/ui/url_strategy_test.dart diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/initialization.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/initialization.dart index 18cb6597f9..115e6ca4f1 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/initialization.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/initialization.dart @@ -8,6 +8,7 @@ import 'dart:js_interop'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; +import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import 'package:web_test_fonts/web_test_fonts.dart'; /// The mode the app is running in. @@ -265,7 +266,7 @@ Future _downloadAssetFonts() async { void _addUrlStrategyListener() { jsSetUrlStrategy = allowInterop((JsUrlStrategy? jsStrategy) { if (jsStrategy == null) { - customUrlStrategy = null; + ui_web.urlStrategy = null; } else { // Because `JSStrategy` could be anything, we check for the // `addPopStateListener` property and throw if it is missing. @@ -274,7 +275,7 @@ void _addUrlStrategyListener() { 'Unexpected JsUrlStrategy: $jsStrategy is missing ' '`addPopStateListener` property'); } - customUrlStrategy = CustomUrlStrategy.fromJs(jsStrategy); + ui_web.urlStrategy = CustomUrlStrategy.fromJs(jsStrategy); } }); registerHotRestartListener(() { diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/window.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/window.dart index 3bf65bb17a..3076ada9d2 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/window.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/window.dart @@ -19,7 +19,6 @@ import 'navigation/history.dart'; import 'navigation/js_url_strategy.dart'; import 'platform_dispatcher.dart'; import 'services.dart'; -import 'test_embedding.dart'; import 'util.dart'; typedef _HandleMessageCallBack = Future Function(); @@ -30,20 +29,6 @@ const bool debugPrintPlatformMessages = false; /// The view ID for the implicit flutter view provided by the platform. const int kImplicitViewId = 0; -/// Whether [_customUrlStrategy] has been set or not. -/// -/// It is valid to set [_customUrlStrategy] to null, so we can't use a null -/// check to determine whether it was set or not. We need an extra boolean. -bool _isUrlStrategySet = false; - -/// A custom URL strategy set by the app before running. -ui_web.UrlStrategy? _customUrlStrategy; -set customUrlStrategy(ui_web.UrlStrategy? strategy) { - assert(!_isUrlStrategySet, 'Cannot set URL strategy more than once.'); - _isUrlStrategySet = true; - _customUrlStrategy = strategy; -} - class EngineFlutterDisplay extends ui.Display { EngineFlutterDisplay({ required this.id, @@ -69,8 +54,8 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { platformDispatcher as EnginePlatformDispatcher; engineDispatcher.viewData[viewId] = this; engineDispatcher.windowConfigurations[viewId] = const ViewConfiguration(); - if (_isUrlStrategySet) { - _browserHistory = createHistoryForExistingState(_customUrlStrategy); + if (ui_web.isCustomUrlStrategySet) { + _browserHistory = createHistoryForExistingState(ui_web.urlStrategy); } registerHotRestartListener(() { _browserHistory?.dispose(); @@ -103,11 +88,9 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { } ui_web.UrlStrategy? get _urlStrategyForInitialization { - final ui_web.UrlStrategy? urlStrategy = - _isUrlStrategySet ? _customUrlStrategy : _createDefaultUrlStrategy(); // Prevent any further customization of URL strategy. - _isUrlStrategySet = true; - return urlStrategy; + ui_web.preventCustomUrlStrategy(); + return ui_web.urlStrategy; } BrowserHistory? @@ -166,9 +149,9 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { ui_web.UrlStrategy? strategy, { required bool useSingle, }) async { - // Prevent any further customization of URL strategy. - _isUrlStrategySet = true; await _browserHistory?.tearDown(); + + ui_web.urlStrategy = strategy; if (useSingle) { _browserHistory = SingleEntryBrowserHistory(urlStrategy: strategy); } else { @@ -179,9 +162,7 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { Future resetHistory() async { await _browserHistory?.tearDown(); _browserHistory = null; - // Reset the globals too. - _isUrlStrategySet = false; - _customUrlStrategy = null; + ui_web.debugResetCustomUrlStrategy(); } Future _endOfTheLine = Future.value(); @@ -370,19 +351,12 @@ typedef _JsSetUrlStrategy = void Function(JsUrlStrategy?); /// A JavaScript hook to customize the URL strategy of a Flutter app. // -// Keep this js name in sync with flutter_web_plugins. Find it at: -// https://github.com/flutter/flutter/blob/custom_location_strategy/packages/flutter_web_plugins/lib/src/navigation/js_url_strategy.dart +// DO NOT CHANGE THE JS NAME, IT IS PUBLIC API AT THIS POINT. // // TODO(mdebbar): Add integration test https://github.com/flutter/flutter/issues/66852 @JS('_flutter_web_set_location_strategy') external set jsSetUrlStrategy(_JsSetUrlStrategy? newJsSetUrlStrategy); -ui_web.UrlStrategy? _createDefaultUrlStrategy() { - return ui.debugEmulateFlutterTesterEnvironment - ? TestUrlStrategy.fromEntry(const TestHistoryEntry('default', null, '/')) - : const ui_web.HashUrlStrategy(); -} - /// The Web implementation of [ui.SingletonFlutterWindow]. class EngineSingletonFlutterWindow extends EngineFlutterWindow { EngineSingletonFlutterWindow( diff --git a/engine/src/flutter/lib/web_ui/lib/ui_web/src/ui_web/navigation/url_strategy.dart b/engine/src/flutter/lib/web_ui/lib/ui_web/src/ui_web/navigation/url_strategy.dart index 4e87fef4c9..689e39d753 100644 --- a/engine/src/flutter/lib/web_ui/lib/ui_web/src/ui_web/navigation/url_strategy.dart +++ b/engine/src/flutter/lib/web_ui/lib/ui_web/src/ui_web/navigation/url_strategy.dart @@ -4,11 +4,47 @@ import 'dart:async'; +import 'package:meta/meta.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; import 'platform_location.dart'; +UrlStrategy _realDefaultUrlStrategy = ui.debugEmulateFlutterTesterEnvironment + ? TestUrlStrategy.fromEntry(const TestHistoryEntry('default', null, '/')) + : const HashUrlStrategy(); + +UrlStrategy get _defaultUrlStrategy => debugDefaultUrlStrategyOverride ?? _realDefaultUrlStrategy; + +/// Overrides the default URL strategy. +/// +/// Setting this to null allows the real default URL strategy to be used. +/// +/// This is intended to be used for testing and debugging only. +@visibleForTesting +UrlStrategy? debugDefaultUrlStrategyOverride; + +/// Whether a custom URL strategy has been set or not. +// +// It is valid to set [_customUrlStrategy] to null, so we can't use a null +// check to determine whether it was set or not. We need an extra boolean. +bool isCustomUrlStrategySet = false; + +/// Whether a custom URL strategy can be set or not. +/// +/// This is used to prevent setting a custom URL strategy for a second time or +/// after the app has already started running. +bool _customUrlStrategyCanBeSet = true; + +/// A custom URL strategy set by the app before running. +UrlStrategy? _customUrlStrategy; + +/// Returns the present [UrlStrategy] for handling the browser URL. +/// +/// Returns null when the browser integration has been manually disabled. +UrlStrategy? get urlStrategy => + isCustomUrlStrategySet ? _customUrlStrategy : _defaultUrlStrategy; + /// Sets a custom URL strategy instead of the default one. /// /// Passing null disables browser history integration altogether. @@ -16,7 +52,27 @@ import 'platform_location.dart'; /// This setter can only be called once. Subsequent calls will throw an error /// in debug mode. set urlStrategy(UrlStrategy? strategy) { - customUrlStrategy = strategy; + assert( + _customUrlStrategyCanBeSet, + 'Cannot set URL strategy a second time or after the app has been initialized.', + ); + preventCustomUrlStrategy(); + isCustomUrlStrategySet = true; + _customUrlStrategy = strategy; +} + +/// From this point on, prevents setting a custom URL strategy. +void preventCustomUrlStrategy() { + _customUrlStrategyCanBeSet = false; +} + +/// Resets everything to do with custom URL strategy. +/// +/// This should only be used in tests to reset things back after each test. +void debugResetCustomUrlStrategy() { + isCustomUrlStrategySet = false; + _customUrlStrategyCanBeSet = true; + _customUrlStrategy = null; } /// Callback that receives the new state of the browser history entry. diff --git a/engine/src/flutter/lib/web_ui/test/ui/url_strategy_test.dart b/engine/src/flutter/lib/web_ui/test/ui/url_strategy_test.dart new file mode 100644 index 0000000000..46299accba --- /dev/null +++ b/engine/src/flutter/lib/web_ui/test/ui/url_strategy_test.dart @@ -0,0 +1,61 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:test/bootstrap/browser.dart'; +import 'package:test/test.dart' hide TypeMatcher, isInstanceOf; +import 'package:ui/src/engine.dart'; +import 'package:ui/ui_web/src/ui_web.dart' as ui_web; + +import '../common/matchers.dart'; +import '../common/test_initialization.dart'; + +void main() { + internalBootstrapBrowserTest(() => testMain); +} + +Future testMain() async { + setUpUnitTests(); + + setUp(() { + ui_web.debugResetCustomUrlStrategy(); + }); + tearDown(() { + ui_web.debugResetCustomUrlStrategy(); + }); + + test('uses the default if no custom URL strategy is set', () { + final ui_web.UrlStrategy defaultUrlStrategy = TestUrlStrategy(); + ui_web.debugDefaultUrlStrategyOverride = defaultUrlStrategy; + + expect(ui_web.urlStrategy, defaultUrlStrategy); + expect(ui_web.isCustomUrlStrategySet, isFalse); + }); + + test('can set a custom URL strategy', () { + final TestUrlStrategy customUrlStrategy = TestUrlStrategy(); + ui_web.urlStrategy = customUrlStrategy; + + expect(ui_web.urlStrategy, customUrlStrategy); + expect(ui_web.isCustomUrlStrategySet, isTrue); + // Does not allow custom URL strategy to be set again. + expect( + () { + ui_web.urlStrategy = customUrlStrategy; + }, + throwsAssertionError, + ); + }); + + test('custom URL strategy can be prevented manually', () { + ui_web.preventCustomUrlStrategy(); + + expect(ui_web.isCustomUrlStrategySet, isFalse); + expect( + () { + ui_web.urlStrategy = TestUrlStrategy(); + }, + throwsAssertionError, + ); + }); +}