Prepare url_launcher for the Link widget

[mdebbar]

Description

• [THIS] Prepare url_launcher for the Link widget.
• [COMPLETE] [web] Implement Link for web ([web] Implement Link for web #3155).
• [COMPLETE] Implement Link for native platforms (Implement Link for native platforms #3177).

The design has been discussed in:

1. https://flutter.dev/go/link-widget
2. https://flutter.dev/go/link-widget-2

Issues:

• Provide a Link widget that works well on the web flutter#59690
• [Web] Hyperlink right click options flutter#44865
• Add a Link widget with option to open url in same window flutter#54281

[ditman]

This looks great! A few questions, but I don't see anything blocking!

Most of the code are very simple classes, but I think a test for the pushRouteNameToFramework is warranted (the isUsingRouter logic, for example)

[ditman]

Is there an imperative Router API that can be called, instead of platform messages?

[mdebbar]

That's a great idea actually.

@chunhtai if I get the Router instance from Router.of(context), is there a way to push the route information name without sending a platform message?

Maybe I can do the same thing with Navigator.of(context).pushNamed(...) when there's no Router?

[chunhtai]

There is no imperative RouterAPI for route update, but you should use SystemNavigator.routeInformationUpdated or SystemNavigator.routeUpdated

[chunhtai]

is the purpose of this function to just update the browser URL bar? calling this method alone will not make navigator/router update their content.

[mdebbar]

The purpose of this function is to tell the framework to push the given route name. This is invoked when a Link widget is pressed. Consequently, the browser URL bar will be updated when the framework sends the routeUpdated/routeInformationUpdated message to the engine.

[mdebbar]

Would it work if I do:

final Router router = Router.of(context);
if (router != null) {
  router.routeInformationProvider.value = {'location': routeName, 'state': null};
} else {
  Navigator.pushNamed(context, routeName);
}

?

[chunhtai]

It wouldn't work for either case, for router the routeInformationProvider is a value listenable, it does not have a setter.
For navigator, it wouldn't work if it has nested navigator, you can probably get the root navigator, but it is not systematic correct.

The best way to do this is somehow trigger didpushRoute/didPushRouteInformation on widgetbindingobserver.
This can be done either by exposing some WidgetBinding API, or send a message through the message channel in the web engine. Besides that you still need to send SystemNavigator.routeInformationUpdated or SystemNavigator.routeUpdated.

[mdebbar]

Ok I got both to work now. For Navigator, I'm just sending a "pushRoute" message to the framework (which goes through binding's didPushRoute). For Router, like you mentioned, I had to send a "pushRouteInformation" message to the framework (which goes through binding's didPushRouteInformation) AND call SystemNavigator.routeInformationUpdated to update the URL in the browser.

[ditman]

Good change, I had a comment about target being any String in the previous PR

[ditman]

This also looks like another platform interface method :P

[mdebbar]

Same as my comment above, could you elaborate please? :)

[ditman]

So in the federated plugins, a "PlatformInterface" is an abstract class describing the behavior that each platform must implement so the plugin works there. The Web implementation extends the PlatformInterface class and provides the web implementations, and there's normally a default MethodChannel implementation that is used by the native side.

The way you override the linkDelegate, and (especially) the builder inside LinkInfo, reminds me of the pattern above (but you reworked it from scratch), that's what I meant by "these look like PlatformInterface" classes, you're using the same pattern, but without leveraging the existing classes :P

[mdebbar]

You're right about link delegate. There's one difference though that makes it not fit exactly well in the platform interface pattern. The default link delegate implementation has to live in url_launcher (the main package) because it needs to call the real canLaunch() and launch(). I'll see what I can do to use the platform interface pattern as much as possible.

OTOH, LinkInfo isn't being overriden anywhere. It's a class that defines the contract between Link and LinkDelegate. It's common between url_launcher and url_launcher_web that's why I put it in the platform interface package.

[ditman]

About canLaunch/launch, if this ever makes it into the framework, it won't be able to use the url_launcher plugin, right? Then it'll need to be copied internally or exposed somehow. Anyway, I don't see anything blocking. (Note that there's an incoming null-safety migration to the package that might cause some merge conflicts with this PR.)

[chunhtai]

This synthesizes the engine sending the message to framework to push the new route. For navigator, this should be sufficient, but for router, it will think this push route information is coming from the engine, so the engine must be up to date with the URL and browser history entry. Thus, it will not send routeInformationUpdate to the engine.
But in your case, you will still need to update the url/browser history entry by sending the routeInformationUpdated

[mdebbar]

I can confirm the behavior you are describing for Navigator. I haven't tried the Router yet. Is this a good app to test with (flutter/flutter#63424) ?

[chunhtai]

yes, that app should be a good example

[ditman]

Other than my comments, I think this looks good!

[ditman]

This is defined as:

  @override
  LinkDelegate linkDelegate = (LinkInfo linkInfo) => WebLinkDelegate(linkInfo);

In the web implementation, how can you override a getter with the function above? Should the signature here be more explicit?

[chunhtai]

LGTM

[Mravuri96]

This should not have been pushed to pub. It's causing build failures.
flutter/flutter#68545

[ditman]

Closing the loop here: Mouad published the fix for the build issue. Apologies for the breakage! /cc @Mravuri96