flutter / uxr

UXR work for Flutter
BSD 3-Clause "New" or "Revised" License
227 stars 28 forks source link

Add code snippets and update comparative analysis table for Navi #38

Closed xuanswe closed 3 years ago

xuanswe commented 3 years ago

all scenarios are covered, except the nested route, which doesn't have an original code to refactor yet.

xuanswe commented 3 years ago

FYI, in the next version 0.2.0, even not 100% sure yet, but I will change the API of RouteStack a bit.

The main idea is still the same:

Changes will be:

Draft idea:

    RouteStack<YourState>(
      pages: (context, state) {
        return [
          NaviPage(
            route: 'page1',
            key: ValueKey(state),
            child: Page1(),
            pageBuilder: (key, child) => CustomPage(key: key, child: child),
          ),
          NaviPage.material(
            route: 'page2',
            key: ValueKey(state),
            child: Page2(),
          ),
        ];
      },
      updateStateOnNewRoute: (routeInfo) {},
    )

Navigate could be smt like this:

context.navi.to('/a/b/c'); // navigate to absolute URL from root stack
context.navi.to('a/b/c'); // navigate relatively from current URL
context.navi.parent.to('/a/b/c'); navigate relatively from current parent URL
context.navi.stack(MyStackMarker()).to('/a/b/c'); navigate relatively from current URL of any stack, if a stack marker is known
xuanswe commented 3 years ago

I have a question, could you maybe help me?

Is there a way to update current URL without rebuild the root RouterDelegate?

The way Navi works is that, it knows the deepest nested route and at the time the deepest nested route finish the build, it already know what is the final URL by merging all routes in the tree. So now it want to tell browser that the correct URL should be changed if it's not the same as the original requested URL. How can I tell browser doing this without calling notifyListeners() in the root RouterDelegate?

CC: @johnpryan @chunhtai

tomgilder commented 3 years ago

@nguyenxndaidev this was something I struggled with, there are a few ways to do it, but it's a good example of the Router API being complicated.

If you get the RouteInformationProvider from the Router you can update it that way:

Router.of(context)
          .routeInformationProvider!
          .routerReportsNewRouteInformation(RouteInformation(location: 'route'));

The default PlatformRouteInformationProvider calls the static SystemNavigator.routeInformationUpdated(location, state), which is another way to do it.

Plus there's the bonus Router.navigate(context, callback) which forces a history item to be created.

xuanswe commented 3 years ago

Thanks @tomgilder, I will try your solution.

xuanswe commented 3 years ago

@tomgilder it works! cool!

InMatrix commented 3 years ago

Thanks for submitting this PR, @nguyenxndaidev. I started looking into the samples you've provided. There is a small detail I noticed with the deep linking by parameter path example. When opening a page with a deep link in the browser, there was still a transition animation as if I entered from a previous page that didn't really exist in this case (see the screen recording below).

https://user-images.githubusercontent.com/348942/114916980-4b7f5700-9dda-11eb-8f57-15223bbf4b5c.mov

Is there a way to disable the animation when the page was opened via a deep link?

xuanswe commented 3 years ago

@InMatrix thank you for checking my PR!

In 0.1.x, my plan is to prove the architecture in general and not yet go into details. The most important of 0.1.x is to see if it will fit for large scale app with just a simple API or not. And it proved that it does and will lead to the next version 0.2.x.

The problem you mentioned is something I know and I think I also know why. I plan to fix it in 0.2.x together with the new changes in the API. Thing is that, 0.2.x will have a totally new implementation, so it's redundant to fix it in 0.1.x.

The main goal of 0.2.x is to make sure it works with complex nested routes.

Do you think is it ok to wait 0.2.x or should I fix this transition problem in 0.1.x for this PR?

The problem is that I only have 3 days / week for this project and I am currently concentrating on 0.2.x, which is also quite urgent because #10 is starting.

InMatrix commented 3 years ago

@nguyenxndaidev Thank you for your prompt response. I'd like to not force you to rush your implementation because of our research schedule. We're going to start with packages that have been out there for some time in our next round of evaluation. The full rationale is described here: https://github.com/flutter/uxr/issues/10#issuecomment-820669779. This might be disappointing to you, but I hope the evaluation results of more mature packages can inform your API design work. I appreciate your engagement with this research project so far.

xuanswe commented 3 years ago

No problem, @InMatrix!

Even if Navi is not chosen for the official analysis because it joined the party too late, I still believe that it's distinct approach will stand out when I create more examples and articles in the next months so people can start to compare Navi to other approaches by themselves.

The question is, will flutter team still review and merge PRs with new code snippets for other packages? I think, users still need to compare code snippets of chosen packages and other packages on their own, right?

xuanswe commented 3 years ago

@InMatrix FYI, I basically made a working complex nested routes with all needed features:

Other notes:

But don't have enough time to release today as planned. I will release 0.2.0 soon in the next days and then update code snippets for all scenarios.

Below is the new API snippets, if you are curious :)

// rename to NaviStack widget and NaviPage for API consistency.
NaviStack(
  pages: () => [
    NaviPage.material(
      key: ValueKey('Home'),
      route: NaviRoute(path: ['/home']),
      child: HomePage(),
    ),
  ],
)
@override
void didChangeDependencies() {
  super.didChangeDependencies();

  context.navi.unprocessedRouteNotifier.addListener(() {
    // unprocessedRoute is the URL part, which is not processed by the parent NaviStack widget.
    // So it should be processed by child/nested NaviStack.
    final unprocessedRoute = context.navi.unprocessedRouteNotifier.route;

    // update your widget state from unprocessed route
  });
}
InMatrix commented 3 years ago

@nguyenxndaidev Thank you for your understanding.

The question is, will flutter team still review and merge PRs with new code snippets for other packages? I think, users still need to compare code snippets of chosen packages and other packages on their own, right?

I certainly agree allowing users to compare code snippets across routing packages they're interested in is valuable. Reviewing them is currently a bottleneck, since we only have a fraction of @chunhtai and @johnpryan's time dedicated to this project. We might need to make some changes to scale this operation, including creating a checklist for the PR author to pre-review their snippets and onboarding a few snippet reviewers from the community.

xuanswe commented 3 years ago

I messed up this PR. Will create another PR later.

xuanswe commented 3 years ago

@InMatrix I create a new PR https://github.com/flutter/uxr/pull/44 for Navi 0.2.0.