Skip to content
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.

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

setInitialRoute is broken for iOS add-to-app #59895

Closed
mehmetf opened this issue Jun 19, 2020 · 22 comments · Fixed by flutter/engine#19684 or flutter/engine#20468
Closed

setInitialRoute is broken for iOS add-to-app #59895

mehmetf opened this issue Jun 19, 2020 · 22 comments · Fixed by flutter/engine#19684 or flutter/engine#20468
Assignees
Labels
a: existing-apps Integration with existing apps via the add-to-app flow c: regression It was better in the past than it is now engine flutter/engine repository. See also e: labels. P0 Critical issues such as a build break or regression platform-ios iOS applications specifically waiting for PR to land (fixed) A fix is in flight

Comments

@mehmetf
Copy link
Contributor

mehmetf commented Jun 19, 2020

b/159359456

https://flutter.dev/docs/development/add-to-app/ios/add-flutter-screen?tab=entrypoint-library-swift-tab#route recommends

FlutterEngine *flutterEngine =
    [[FlutterEngine alloc] initWithName:@"my flutter engine"];
[[flutterEngine navigationChannel] invokeMethod:@"setInitialRoute"
                                      arguments:@"/onboarding"];
[flutterEngine run];

This does not work. The initial route comes out as default ("/"). If I initialize view controller with project (instead of engine) and use [flutterViewController setInitialRoute], it works.

@mehmetf mehmetf added c: regression It was better in the past than it is now platform-ios iOS applications specifically a: existing-apps Integration with existing apps via the add-to-app flow customer: merch (g3) labels Jun 19, 2020
@Hixie Hixie added the engine flutter/engine repository. See also e: labels. label Jun 23, 2020
@Hixie
Copy link
Contributor

Hixie commented Jun 23, 2020

@chinmaygarde says this is more or less working as intended and that there is a workaround so I will reduce it to P3; he will update this bug shortly.

@Hixie Hixie added P1 High-priority issues at the top of the work list and removed P2 labels Jun 23, 2020
@mehmetf mehmetf added the P1 label Jul 2, 2020
@chinmaygarde chinmaygarde removed the P1 High-priority issues at the top of the work list label Jul 6, 2020
@chinmaygarde chinmaygarde self-assigned this Jul 6, 2020
@chinmaygarde chinmaygarde added this to the 1.21 - July 2020 milestone Jul 6, 2020
@chinmaygarde
Copy link
Member

@mehmetf : So, I am not sure how the initial recommendation on the page you linked was made. It is definitely inaccurate. Channel setup has always occurred when the underlying shell in the engine is setup. In the FlutterEngine API, the shell isn't setup till the runWithEntrypoint:libraryURI: method is called. So, I am pretty sure [flutterEngine navigationChannel] result is nil. Looking back at the history, it seems to have always worked this way with the FlutterViewController being special cased to setup the shell (and it channels) upfront to specifically work around the issue of the navigation channel being unavailable. The fix is to do the same in FlutterEngine (i.e, create the shell with Settings object in the FlutterDartProject).

The behavior described in the docs is a fair ask but it is a bit mystifying how that recommendation was made when the implementation has never worked that way. In any case, I see you bumped up the priority of this to P1. Is this now a blocker? My reading of the internal issue indicated that there was a workaround. I'll have reprioritize some other stuff if this is at indicated priority.

@mehmetf
Copy link
Contributor Author

mehmetf commented Jul 7, 2020

It is definitely inaccurate.

FWIW, I was very surprised at that recommendation as well.

Are you referring to initialize view controller with project instead of engine workaround? If so, that does not work for the user since they are an add-to-app user who wants to control how/when engine is initialized.

However, it is not a launch blocker for them for now so lowering priority to P2. We should, at the very least, fix documentation asap tho.

@mehmetf mehmetf added P2 and removed P1 labels Jul 7, 2020
@chinmaygarde
Copy link
Member

Are you referring to initialize view controller with project instead of engine workaround? If so, that does not work...

Yes. Ok. This would be a blocker for them eventually because there is no way in the current API to send a message before the run call. The UIViewController manages this today because it makes a private call to the engine to create its shell and setup channels. I'll find an owner for this in the next triage run.

@chinmaygarde chinmaygarde removed their assignment Jul 7, 2020
@xster xster self-assigned this Jul 8, 2020
@xster xster added this to Awaiting triage in Mobile - Add-to-app review via automation Jul 8, 2020
@xster xster moved this from Awaiting triage to Engineer reviewed in Mobile - Add-to-app review Jul 8, 2020
@xster
Copy link
Member

xster commented Jul 11, 2020

ah yes, the docs are totally off. I'm surprised no one ran into this so far.

We have this on Android https://flutter.dev/docs/development/add-to-app/android/add-flutter-screen#initial-route-with-a-cached-engine and implemented channel buffers to fix timing issues flutter/engine#12167 but we didn't change the loading sequence on iOS.

Haven't decided how to fix this yet but I'm tempted to fix the loading sequence discrepancy between Android and iOS which causes unnecessary confusion https://flutter.dev/docs/development/add-to-app/performance#starting-the-dart-vm.

@xster
Copy link
Member

xster commented Jul 11, 2020

Actually that doesn't also fix the other root issue that the whole setInitialRoute API is incredibly confusing. I think we made slight progress in making it less of a footgun building it into the FlutterActivity builder pattern. I'm going to repeat the same thing here:

1- Deprecate the FlutterViewController setInitialRoute API which needs to be called at a (very fleeting) specific moment.
2- Add a constructor to iOS FlutterEngine and FlutterViewController to save an initial route to be used when running.
3- Once you set an initial route, you can never change it. There may be valid cases but it's already incredibly difficult to pull off today. On UIViewController/Activities, there's theoretically no time between creation and displaying it (at which point the initial route is "committed") to change it. On FlutterEngine, there's also no strong reasons to construct an engine and not run it (at which point the initial route is "committed"). There will continue to be documentation for how to push/pop the route on a cached engine on Android and iOS.

This fixes 2 issues with one code change.

@chinmaygarde
Copy link
Member

@xster I am bumping the milestone for this as the month is out. Please re-adjust as necessary if you think the linked patch cannot land by then.

@xster
Copy link
Member

xster commented Aug 3, 2020

yup, thanks

@xster
Copy link
Member

xster commented Aug 11, 2020

mini update: about the land fix PR

@lds8988
Copy link

lds8988 commented Aug 12, 2020

@xster when this fix will merge to stable channel?

@xster
Copy link
Member

xster commented Aug 12, 2020

Did you mean 1.21? We're currently not planning to. Are you blocked on this issue?

@lds8988
Copy link

lds8988 commented Aug 13, 2020

@xster yes. My app's feature should jump to flutter page from native page, but setInitialRoute not work so that can not jump to specified page.Is it possible release a 1.20.2 to fix this bug?

@xster xster reopened this Aug 13, 2020
Mobile - Add-to-app review automation moved this from Engineer reviewed to Awaiting triage Aug 13, 2020
@xster
Copy link
Member

xster commented Aug 13, 2020

Reverted. Reworking tests.

@xster
Copy link
Member

xster commented Aug 13, 2020

@lds8988 it's a non-trivial refactor so we're not going to make a cherry pick on the stable branch. Please wait for the 1.22 release.

In the meantime, if you're blocked, consider the following workarounds:

  1. Use a Dart entrypoint to customize the initial Flutter screen https://flutter.dev/docs/development/add-to-app/ios/add-flutter-screen?tab=engine-swift-tab#dart-entrypoint.
  2. Use [[flutterEngine navigationChannel] invokeMethod:@"pushRoute" arguments:route]; if needed. This creates 2 routes instead of 1 in Flutter but you can control the foremost visible route.

@lds8988
Copy link

lds8988 commented Aug 13, 2020

@xster I'll try it. Thx for reply.

Mobile - Add-to-app review automation moved this from Awaiting triage to Engineer reviewed Aug 13, 2020
@murtuzasaify
Copy link

@xster I am facing the same problem, setting initial route on engines navigation channel has no impact and eventually had to navigate to desired screen without warming up the engine.
Any info on when the pull request for handling initial route on engine be merged and available ?

@xster
Copy link
Member

xster commented Aug 17, 2020

It's already on master. I'll add more documentation on it once it's on stable channel in about a month. In the meantime, you can probably figure out how to use the new APIs from the headers. (e.g. use the specific run selectors on the FlutterEngine to specify the initial route when running the engine initially).

@murtuzasaify
Copy link

thanks @xster , I can see that you introduced - (BOOL)runWithEntrypoint:(NSString*)entrypoint initialRoute:(NSString*)initialRoute which can now be used to set initial route.
How can I consume and test this currently, if I point to master branch of Flutter pod (pod Flutter, branch: master) will this be available ?

@xster
Copy link
Member

xster commented Aug 19, 2020

No, you should flutter channel master flutter upgrade instead if you want to try this on master.

@jzyds
Copy link

jzyds commented Aug 24, 2020

Is this fixed already in 1.22.0-1.0.pre?

How to initialRoute with arguments?

@Senasiko
Copy link

@lds8988 it's a non-trivial refactor so we're not going to make a cherry pick on the stable branch. Please wait for the 1.22 release.

In the meantime, if you're blocked, consider the following workarounds:

  1. Use a Dart entrypoint to customize the initial Flutter screen https://flutter.dev/docs/development/add-to-app/ios/add-flutter-screen?tab=engine-swift-tab#dart-entrypoint.
  2. Use [[flutterEngine navigationChannel] invokeMethod:@"pushRoute" arguments:route]; if needed. This creates 2 routes instead of 1 in Flutter but you can control the foremost visible route.

What should I do to control the foremost visible route? If I use [[flutterEngine navigationChannel] invokeMethod:@"pushRoute" arguments:route]; and Navigator.pop() in flutter view, I will go back to the initialRoute or home, not the view in app.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
@flutter-triage-bot flutter-triage-bot bot added P0 Critical issues such as a build break or regression and removed P2 labels Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: existing-apps Integration with existing apps via the add-to-app flow c: regression It was better in the past than it is now engine flutter/engine repository. See also e: labels. P0 Critical issues such as a build break or regression platform-ios iOS applications specifically waiting for PR to land (fixed) A fix is in flight
Projects
Mobile - Add-to-app review
  
Engineer reviewed
Development

Successfully merging a pull request may close this issue.

8 participants