From 65b25427f0e694447fa024b2d9d7368d68d49b98 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Wed, 31 May 2023 17:37:18 -0400 Subject: [PATCH] [web] Remove the JS API for url strategy (flutter/engine#42134) Finally, we can remove this JS global function for customizing the url strategy. Why I think we don't need to go through an official deprecation process: 1. It was initially made for internal use in Google3, and right now there are no references to it. 2. There's no public documentation of this JS function. 3. External users customize their url strategy through `flutter_web_plugins` which has been [migrated](https://github.com/flutter/flutter/pull/123443) already. --- .../ci/licenses_golden/licenses_flutter | 6 - .../flutter/lib/web_ui/lib/src/engine.dart | 2 - .../web_ui/lib/src/engine/initialization.dart | 25 ---- .../lib/web_ui/lib/src/engine/navigation.dart | 7 -- .../engine/navigation/js_url_strategy.dart | 87 -------------- .../src/engine/navigation/url_strategy.dart | 48 -------- .../web_ui/lib/src/engine/test_embedding.dart | 2 +- .../lib/web_ui/lib/src/engine/window.dart | 11 -- .../src/ui_web/navigation/url_strategy.dart | 8 +- .../lib/web_ui/test/engine/history_test.dart | 5 +- .../lib/web_ui/test/engine/routing_test.dart | 110 +++++++++++------- 11 files changed, 76 insertions(+), 235 deletions(-) delete mode 100644 engine/src/flutter/lib/web_ui/lib/src/engine/navigation.dart delete mode 100644 engine/src/flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart delete mode 100644 engine/src/flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart diff --git a/engine/src/flutter/ci/licenses_golden/licenses_flutter b/engine/src/flutter/ci/licenses_golden/licenses_flutter index 362ae744bd..834ba5aec7 100644 --- a/engine/src/flutter/ci/licenses_golden/licenses_flutter +++ b/engine/src/flutter/ci/licenses_golden/licenses_flutter @@ -1975,10 +1975,7 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/js_interop/js_typed_data.dart ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/key_map.g.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/mouse_cursor.dart + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/navigation.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/navigation/history.dart + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/noto_font.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/onscreen_logging.dart + ../../../flutter/LICENSE ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/picture.dart + ../../../flutter/LICENSE @@ -4639,10 +4636,7 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/js_interop/js_typed_data.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/key_map.g.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/keyboard_binding.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/mouse_cursor.dart -FILE: ../../../flutter/lib/web_ui/lib/src/engine/navigation.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/navigation/history.dart -FILE: ../../../flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart -FILE: ../../../flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/noto_font.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/onscreen_logging.dart FILE: ../../../flutter/lib/web_ui/lib/src/engine/picture.dart diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine.dart b/engine/src/flutter/lib/web_ui/lib/src/engine.dart index 6fb3113cd9..e5c92bfe21 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine.dart @@ -113,8 +113,6 @@ export 'engine/key_map.g.dart'; export 'engine/keyboard_binding.dart'; export 'engine/mouse_cursor.dart'; export 'engine/navigation/history.dart'; -export 'engine/navigation/js_url_strategy.dart'; -export 'engine/navigation/url_strategy.dart'; export 'engine/noto_font.dart'; export 'engine/onscreen_logging.dart'; export 'engine/picture.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 115e6ca4f1..bf89ca81c3 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,7 +8,6 @@ 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. @@ -133,10 +132,6 @@ Future initializeEngineServices({ // Store `jsConfiguration` so user settings are available to the engine. configuration.setUserConfiguration(jsConfiguration); - // Setup the hook that allows users to customize URL strategy before running - // the app. - _addUrlStrategyListener(); - // Called by the Web runtime just before hot restarting the app. // // This extension cleans up resources that are registered with browser's @@ -263,26 +258,6 @@ Future _downloadAssetFonts() async { } } -void _addUrlStrategyListener() { - jsSetUrlStrategy = allowInterop((JsUrlStrategy? jsStrategy) { - if (jsStrategy == null) { - ui_web.urlStrategy = null; - } else { - // Because `JSStrategy` could be anything, we check for the - // `addPopStateListener` property and throw if it is missing. - if (!hasJsProperty(jsStrategy, 'addPopStateListener')) { - throw StateError( - 'Unexpected JsUrlStrategy: $jsStrategy is missing ' - '`addPopStateListener` property'); - } - ui_web.urlStrategy = CustomUrlStrategy.fromJs(jsStrategy); - } - }); - registerHotRestartListener(() { - jsSetUrlStrategy = null; - }); -} - /// Whether to disable the font fallback system. /// /// We need to disable font fallbacks for some framework tests because diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/navigation.dart deleted file mode 100644 index e3746d1dc9..0000000000 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation.dart +++ /dev/null @@ -1,7 +0,0 @@ -// 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. - -export 'navigation/history.dart'; -export 'navigation/js_url_strategy.dart'; -export 'navigation/url_strategy.dart'; diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart deleted file mode 100644 index 04a4f4392a..0000000000 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/js_url_strategy.dart +++ /dev/null @@ -1,87 +0,0 @@ -// 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. - -@JS() -library js_url_strategy; - -import 'dart:js_interop'; - -import 'package:ui/ui.dart' as ui; - -import '../dom.dart'; - -typedef _PathGetter = String Function(); - -typedef _StateGetter = Object? Function(); - -typedef _AddPopStateListener = ui.VoidCallback Function(DartDomEventListener); - -typedef _StringToString = String Function(String); - -typedef _StateOperation = void Function( - Object? state, String title, String url); - -typedef _HistoryMove = Future Function(double count); - -/// The JavaScript representation of a URL strategy. -/// -/// This is used to pass URL strategy implementations across a JS-interop -/// bridge from the app to the engine. -@JS() -@anonymous -@staticInterop -abstract class JsUrlStrategy { - /// Creates an instance of [JsUrlStrategy] from a bag of URL strategy - /// functions. - external factory JsUrlStrategy({ - required _PathGetter getPath, - required _StateGetter getState, - required _AddPopStateListener addPopStateListener, - required _StringToString prepareExternalUrl, - required _StateOperation pushState, - required _StateOperation replaceState, - required _HistoryMove go, - }); -} - -extension JsUrlStrategyExtension on JsUrlStrategy { - /// Adds a listener to the `popstate` event and returns a function that, when - /// invoked, removes the listener. - external ui.VoidCallback addPopStateListener(DartDomEventListener fn); - - /// Returns the active path in the browser. - external String getPath(); - - /// Returns the history state in the browser. - /// - /// See: https://developer.mozilla.org/en-US/docs/Web/API/History/state - external Object? getState(); - - /// Given a path that's internal to the app, create the external url that - /// will be used in the browser. - external String prepareExternalUrl(String internalUrl); - - /// Push a new history entry. - /// - /// See: https://developer.mozilla.org/en-US/docs/Web/API/History/pushState - external void pushState(Object? state, String title, String url); - - /// Replace the currently active history entry. - /// - /// See: https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState - external void replaceState(Object? state, String title, String url); - - /// Moves forwards or backwards through the history stack. - /// - /// A negative [count] value causes a backward move in the history stack. And - /// a positive [count] value causs a forward move. - /// - /// Examples: - /// - /// * `go(-2)` moves back 2 steps in history. - /// * `go(3)` moves forward 3 steps in hisotry. - /// - /// See: https://developer.mozilla.org/en-US/docs/Web/API/History/go - external Future go(double count); -} diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart deleted file mode 100644 index 17453b0d26..0000000000 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/navigation/url_strategy.dart +++ /dev/null @@ -1,48 +0,0 @@ -// 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 'dart:async'; - -import 'package:ui/ui.dart' as ui; -import 'package:ui/ui_web/src/ui_web.dart' as ui_web; - -import '../dom.dart'; -import '../safe_browser_api.dart'; -import 'js_url_strategy.dart'; - -/// Wraps a custom implementation of [ui_web.UrlStrategy] that was previously converted -/// to a [JsUrlStrategy]. -class CustomUrlStrategy extends ui_web.UrlStrategy { - /// Wraps the [delegate] in a [CustomUrlStrategy] instance. - CustomUrlStrategy.fromJs(this.delegate); - - final JsUrlStrategy delegate; - - @override - ui.VoidCallback addPopStateListener(ui_web.PopStateListener fn) => - delegate.addPopStateListener(allowInterop((DomEvent event) => - fn((event as DomPopStateEvent).state) - )); - - @override - String getPath() => delegate.getPath(); - - @override - Object? getState() => delegate.getState(); - - @override - String prepareExternalUrl(String internalUrl) => - delegate.prepareExternalUrl(internalUrl); - - @override - void pushState(Object? state, String title, String url) => - delegate.pushState(state, title, url); - - @override - void replaceState(Object? state, String title, String url) => - delegate.replaceState(state, title, url); - - @override - Future go(int count) => delegate.go(count.toDouble()); -} diff --git a/engine/src/flutter/lib/web_ui/lib/src/engine/test_embedding.dart b/engine/src/flutter/lib/web_ui/lib/src/engine/test_embedding.dart index 18febe3f23..ba5647c8df 100644 --- a/engine/src/flutter/lib/web_ui/lib/src/engine/test_embedding.dart +++ b/engine/src/flutter/lib/web_ui/lib/src/engine/test_embedding.dart @@ -30,7 +30,7 @@ class TestHistoryEntry { /// /// It keeps a list of history entries and event listeners in memory and /// manipulates them in order to achieve the desired functionality. -class TestUrlStrategy extends ui_web.UrlStrategy { +class TestUrlStrategy implements ui_web.UrlStrategy { /// Creates a instance of [TestUrlStrategy] with an empty string as the /// path. factory TestUrlStrategy() => TestUrlStrategy.fromEntry(const TestHistoryEntry(null, null, '')); 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 1370d45802..8785ce356b 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 @@ -16,7 +16,6 @@ import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import '../engine.dart' show DimensionsProvider, registerHotRestartListener, renderer; import 'dom.dart'; import 'navigation/history.dart'; -import 'navigation/js_url_strategy.dart'; import 'platform_dispatcher.dart'; import 'services.dart'; import 'util.dart'; @@ -324,16 +323,6 @@ class EngineFlutterWindow extends ui.SingletonFlutterWindow { ui.Size? webOnlyDebugPhysicalSizeOverride; } -typedef _JsSetUrlStrategy = void Function(JsUrlStrategy?); - -/// A JavaScript hook to customize the URL strategy of a Flutter app. -// -// 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); - /// 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 9e509de073..df8b6f2174 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 @@ -82,11 +82,7 @@ typedef PopStateListener = void Function(Object? state); /// /// By default, the [HashUrlStrategy] subclass is used if the app doesn't /// specify one. -abstract class UrlStrategy { - /// Abstract const constructor. This constructor enables subclasses to provide - /// const constructors so that they can be used in const expressions. - const UrlStrategy(); - +abstract interface class UrlStrategy { /// Adds a listener to the `popstate` event and returns a function that, when /// invoked, removes the listener. ui.VoidCallback addPopStateListener(PopStateListener fn); @@ -139,7 +135,7 @@ abstract class UrlStrategy { /// // Somewhere before calling `runApp()` do: /// setUrlStrategy(const HashUrlStrategy()); /// ``` -class HashUrlStrategy extends UrlStrategy { +class HashUrlStrategy implements UrlStrategy { /// Creates an instance of [HashUrlStrategy]. /// /// The [PlatformLocation] parameter is useful for testing to mock out browser diff --git a/engine/src/flutter/lib/web_ui/test/engine/history_test.dart b/engine/src/flutter/lib/web_ui/test/engine/history_test.dart index 9cba044620..b661385e1d 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/history_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/history_test.dart @@ -8,9 +8,8 @@ import 'package:quiver/testing/async.dart'; import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart' show window; -import 'package:ui/src/engine/dom.dart' - show DomEvent, createDomPopStateEvent; -import 'package:ui/src/engine/navigation.dart'; +import 'package:ui/src/engine/dom.dart' show DomEvent, createDomPopStateEvent; +import 'package:ui/src/engine/navigation/history.dart'; import 'package:ui/src/engine/services.dart'; import 'package:ui/src/engine/test_embedding.dart'; import 'package:ui/ui_web/src/ui_web.dart'; diff --git a/engine/src/flutter/lib/web_ui/test/engine/routing_test.dart b/engine/src/flutter/lib/web_ui/test/engine/routing_test.dart index aa1d572102..ed09a11ea9 100644 --- a/engine/src/flutter/lib/web_ui/test/engine/routing_test.dart +++ b/engine/src/flutter/lib/web_ui/test/engine/routing_test.dart @@ -3,12 +3,12 @@ // found in the LICENSE file. import 'dart:async'; -import 'dart:js_util' as js_util; import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; import 'package:ui/src/engine.dart' hide window; import 'package:ui/ui.dart' as ui; +import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import '../common/matchers.dart'; import 'history_test.dart'; @@ -48,27 +48,15 @@ void testMain() { expect(window.viewId, 0); }); - test('window.defaultRouteName should work with JsUrlStrategy', () async { - dynamic state = {}; - final JsUrlStrategy jsUrlStrategy = JsUrlStrategy( - getPath: allowInterop(() => '/initial'), - getState: allowInterop(() => state), - addPopStateListener: allowInterop((DartDomEventListener listener) => allowInterop(() {})), - prepareExternalUrl: allowInterop((String value) => ''), - pushState: allowInterop((Object? newState, String title, String url) { - expect(newState is Map, true); - }), - replaceState: allowInterop((Object? newState, String title, String url) { - expect(newState is Map, true); - state = newState; - }), - go: allowInterop(([double? delta]) async { - expect(delta, -1); - })); - final CustomUrlStrategy strategy = - CustomUrlStrategy.fromJs(jsUrlStrategy); - await window.debugInitializeHistory(strategy, useSingle: true); + test('window.defaultRouteName should work with a custom url strategy', () async { + const String path = '/initial'; + const Object state = {'origin': true}; + + final _SampleUrlStrategy customStrategy = _SampleUrlStrategy(path, state); + await window.debugInitializeHistory(customStrategy, useSingle: true); expect(window.defaultRouteName, '/initial'); + // Also make sure that the custom url strategy was actually used. + expect(customStrategy.wasUsed, isTrue); }); test('window.defaultRouteName should not change', () async { @@ -441,7 +429,12 @@ void testMain() { test('can disable location strategy', () async { // Disable URL strategy. - expect(() => jsSetUrlStrategy(null), returnsNormally); + expect( + () { + ui_web.urlStrategy = null; + }, + returnsNormally, + ); // History should be initialized. expect(window.browserHistory, isNotNull); // But without a URL strategy. @@ -455,27 +448,35 @@ void testMain() { expect(window.browserHistory.currentPath, '/'); }); - test('js interop throws on wrong type', () { - expect(() => jsSetUrlStrategy(123), throwsA(anything)); - expect(() => jsSetUrlStrategy('foo'), throwsA(anything)); - expect(() => jsSetUrlStrategy(false), throwsA(anything)); - expect(() => jsSetUrlStrategy([]), throwsA(anything)); - }); - - test('cannot set url strategy after it is initialized', () async { + test('cannot set url strategy after it was initialized', () async { final TestUrlStrategy testStrategy = TestUrlStrategy.fromEntry( const TestHistoryEntry('initial state', null, '/'), ); await window.debugInitializeHistory(testStrategy, useSingle: true); - expect(() => jsSetUrlStrategy(null), throwsA(isAssertionError)); + expect( + () { + ui_web.urlStrategy = null; + }, + throwsA(isAssertionError), + ); }); test('cannot set url strategy more than once', () async { // First time is okay. - expect(() => jsSetUrlStrategy(null), returnsNormally); + expect( + () { + ui_web.urlStrategy = null; + }, + returnsNormally, + ); // Second time is not allowed. - expect(() => jsSetUrlStrategy(null), throwsA(isAssertionError)); + expect( + () { + ui_web.urlStrategy = null; + }, + throwsA(isAssertionError), + ); }); // Regression test for https://github.com/flutter/flutter/issues/77817 @@ -486,10 +487,41 @@ void testMain() { }); } -void jsSetUrlStrategy(dynamic strategy) { - js_util.callMethod( - domWindow, - '_flutter_web_set_location_strategy', - [strategy], - ); +class _SampleUrlStrategy implements ui_web.UrlStrategy { + _SampleUrlStrategy(this._path, this._state); + + final String _path; + final Object? _state; + + bool wasUsed = false; + + @override + String getPath() => _path; + + @override + Object? getState() => _state; + + @override + ui.VoidCallback addPopStateListener(DartDomEventListener listener) { + wasUsed = true; + return () {}; + } + + @override + String prepareExternalUrl(String value) => ''; + + @override + void pushState(Object? newState, String title, String url) { + wasUsed = true; + } + + @override + void replaceState(Object? newState, String title, String url) { + wasUsed = true; + } + + @override + Future go(int delta) async { + wasUsed = true; + } }