aurelia / router

A powerful client-side router.
MIT License
120 stars 115 forks source link

Feature request: passing state when navigating #479

Closed jwx closed 6 years ago

jwx commented 7 years ago

I'm submitting a feature request

A somewhat common scenario (at least for me) is the desire to pass data/a state when navigating from one view to another. This state could for example be the name of previous view, selected sort order or some json data that's been fetched. The solution (for me, most of the time) is to pass the state through an injected singleton and while this state singleton is useful for global state or when additional functionality is required, it just feels cumbersome for passing state when navigating.

I propose adding a new parameter, state: any, to navigate and navigateToRoute that will be transfered into currentInstruction so that the state is available in the new view's activate function.

In addition, I propose expanding navigate to be able to take an url in the same format that is used when configuring routes so that it's possible to get url generation in navigate with the aid of an additional new parameter, parameters: any (mirroring the parameters for navigateToRoute).

I'm currently doing variations/patches of the above in my projects so if this is something you want, I'd be willing to create the PR.

jwx commented 7 years ago

@EisenbergEffect

EisenbergEffect commented 7 years ago

I've generally been opposed to this because it means that there's some aspect of the history which will break when the browser is refreshed or when a link is shared. Can you talk more specifics about the scenario where you don't think these issues would be a concern?

jwx commented 7 years ago

It's intended for data that's either non-essential for functionality or that's fetched if not present. I've seen the question on how to pass into the next view on gitter a couple of times and the suggestions tend to be state singleton (which presents the same problems when it comes to links and refresh).

An example I have is from a mobile app where selecting an item in a list of items navigates to /item/:id/ and passes the item data object from the list. The target view then initiates a fetch based on the id, but presents the item based on the passed object, if there is one, while waiting for the reply. (In some cases, it didn't even perform the fetch if the data was passed along.) It made a difference for the user experience.

(And then, of course, there's the, albeit very few, times when one doesn't care about supporting external links or page refresh.)

shanonvl commented 7 years ago

Hi @jwx -- I've hit this myself when developing apps and wanted to throw this out for consideration in lieu of adding this functionality to Aurelia.

While it's still a simple case, IMO this is the exact use case for a logical data layer ie. separation of concerns wrt data and presentation.

If I’m following correctly, in your activate:

activate(routeConfig, params) {
  const id = params.id,
     allItems = params.allItems,  // assume this magically gets wired to [ { id: 1 }, { id:2},
                                                    // { … ], { id:10 } ]
     theItem = id ? allItems.find(i => i.id === id) : null;

  // if eg. id === 11 then this executes 
  if(!theItem) {
      // run your fetch logic
      // complexity here will vary based on query…  
  }

  // continue normal logic   

}

If this accurately represents your logic, there's going to be a lot of code duplication in each view model that needs to access data, or you’re going to centralize the retrieval logic and import a shared function that does it (almost the same as singleton import already). In my case, I just define a DataService which has a eg. fetch(id1,id2,...) method. This DataService abstracts everything about data caching and retrieval and keeps my views focused on display logic.

There are a variety of ways to apply the DataService to your code. You can use the standard import method which adds a few lines of boilerplate:

import {DataService} from ‘..’;
...
@inject(DataService)
…
constructor(dataService) {
   this.dataService = dataService;
}
…
activate(…) {
   const item = this.dataService.fetch(id);
}

A more advanced solution would be to inject the data service singleton or a query result using a decorator:

import {dataService} from '…';

class MyViewModel {

    @dataService    dataService;

        activate(…)  {
            const id = params.id,
                item = id ? this.dataService.fetch(id) : null;     
            // use the item
        }
}

You could also have the decorator run a query:

import {dataServiceQuery} from '…';

class MyViewModel {

    itemId = null;

        // this will call the query iff a valid itemId exists and call itemChanged whenever 
        // the result changes
        @dataServiceQuery(`SELECT * from ITEMS WHERE ID = '{itemId}'`) item;

        activate(…)  {
            // dataServiceQuery observes itemId because it’s a named parameter in the query.
        this.itemId = params.id || null; 
        }

        deactivate() {
            this.itemId = null;
        }

        itemChanged(item) {
        // data service updated the item … 
        }
}

Either way, decoupling data retrieval from view logic will be very beneficial as the application grows to many view models that need access to data.

From a user experience perspective, all of the above is non-blocking so workarounds may be applied for the UX issue. One solution is to display placeholders (Slack / Facebook / many others do this) while the item is being retrieved. These are swapped out when the item is loaded.

(And then, of course, there's the, albeit very few, times when one doesn't care about supporting external links or page refresh.)

Sure that may work in your specific use case but IMO functionality that breaks external links and refresh should be very carefully reviewed prior to addition to a general framework like Aurelia.

jwx commented 7 years ago

Hi, @shanonvl. Thank you for trying to help. Unfortunately, my posts haven't been clear enough to give you a good picture of the feature request or its scenarios. It doesn't have anything to do specifically with data retrieval and separation of concerns, but rather what features the router support when it's used to navigate from one route to another.

It seems like I used a bad example to try and illustrate scenarios for this. I don't have any code in any project that looks and behaves like your first example since I'm very strict when it comes to separation between data and view layers. I apologize for giving you the wrong impression.

(If I'll try and squeeze your examples into a scenario it would be that an item (or any kind of data) from one view is passed and used as a placeholder according to your second last paragraph.)

While your post didn't quite hit the mark, I think it's a good description of and argument for separation of concerns between data and presentation and will surely be useful to people as such.

And since I've obviously been unclear, I'd just like to point out that adding this functionality doesn't automatically break something. As with a lot of things in Aurelia and other frameworks, it's about how you use the tools.

jwx commented 7 years ago

@EisenbergEffect Another user question on gitter regarding this feature request: "[...] the scenario is that I am routing to a specific page based on specific logic and I am using the router to pass data to the next route. I do this by pushing an object into a custom settings called "data" (settings{data:[]})" of the next route. Then when the view-model is attached for the next route I simply get the object off the router - "this.router.currentInstruction.config.settings.data" - it works really well [...]" @ihorizon

bigopon commented 7 years ago

I was looking for something similar, and I think it would be great to have. The use case is I want to support forward navigation as much as backward navigation. Without a state/ intent passing around betweens routes, i have to populate history

Ex.

When i come to dashboard and need to determine what kind of view user want

/Dashboard/gridview
/Dashboard/listview
...

Its is ok when you go forward, but will be terrible when you go backward, as when my user click the browser back button, they expect to navigate away from dashboard, not switching between view types.

It can be argued that those state can be stored separately in a shared state class instance, butbthen i have to mix up unimportant parts with the important ones.

Maybe these all are because i dont know how to do it properly, but could be good if some1 could point out how to do it.

plwalters commented 7 years ago

I agree with @EisenbergEffect I would not suggest this as it is poor user experience and hard to manage. If you want to achieve this I would highly suggest using the URL which can be bookmarked, saved, navigated, etc... Example -

let options = encodeURIComponent(JSON.stringify(routeOptions));
`http://localhost:9000/#/home?${options}`

This can get long though so you can base64 encode it -

let options = btoa(JSON.stringify({test: 'string'}));
`http://localhost:9000/#/home?${options}`

Which you can decode later when you need it.

jwx commented 7 years ago

I've got to disagree with @PWKad on this. There's nothing within the principles of user experience that says that URLs that can be bookmarked, saved, shared, navigated, etc are good. A mobile or desktop app (achieved with Aurelia and Cordova or Electron) might not expose the URLs at all and still provide a great user experience. User experience is about specific situations and users and doesn't define definitive good and bad technical choices. And I don't really understand what's so hard to manage.

There's different ways to get things done and while we might not like and use some of them that doesn't mean that they're bad per se. Aurelia is generally very good at providing different ways to get the job done. The question here is just whether Aurelia wants to provide this one too. @EisenbergEffect

plwalters commented 7 years ago

UX is not a spec or standard so we don't need to agree on this and that's fine but refreshing a page wouldn't work, anchors would not work, nor would they open in new page, and those are definitely UX items that would be unsupported here. The problem in implementation is that it would be up to the router to maintain a cache of previous routes now instead of relying on the browser. Implementing a state manager should be trivial enough so why not just use that approach instead of bloating the router?

jwx commented 7 years ago

Refreshing the page would indeed not preserve these params and they wouldn't be passed out of the Aurelia app (which I assume also includes the comment about the anchors), but in a mobile or desktop app this might not be part of the UX at all. When it comes to implementation, I think there's some misunderstanding (might be from my side). I don't see the need of a cache in the router. Simply add the parameter to the functions and pass the arguments into the currentInstruction. It will mean some extra lines of code, sure, but I'd hardly call it bloating.

plwalters commented 7 years ago

I guess I'm looking at it from the standpoint of maintenance and bug triage. As soon as that is added and someone hits the back button and their app breaks it's going to be a bug report :) Isolating this down to native applications or other specific scenarios isn't really feasible but there is a settings object on each instruction that could be used or overwritten to accomplish this as well if you weren't worried about history.

If everyone else agrees we should then by all means we should but I'm just not a big fan of this so presenting my opinions on why.

estaylorco commented 7 years ago

@jwx If I could chime in here...

I went through this with the Login portion of my app, where I have to route between Login, ForgotPassword, and Recovery views. There is a more elegant way to do what you're trying to do, I think.

I created a LoginSession singleton that holds state between views (but some functionality as well, according to the Session Façade pattern). When the user logs in successfully finally, I do this:

this.aurelia.setRoot('app/shell/shell').then((aurelia) => {
  this.session.dispose();
  aurelia.container.unregister(LoginSession);
  aurelia.root.viewModel.router.navigateToRoute('home');
});

and at the head of the Login class, I have (the relevant bits):

...
import {Aurelia, transient} from 'aurelia-framework';
import {LoginSession} from 'app/auth/login-session';

@inject(Aurelia, LoginSession)
@transient()
export class Login {
  constructor(aurelia, session) {
    this.aurelia = aurelia;
    this.session = session;
  }
...

In other words, just remove your singleton from the container once that "state" is no longer needed. Taking the singleton Session Façade approach, and advantage of Aurelia's DI, you can prevent the need to spread functionality against that state over two or more views.

Would this pattern work for you?

jwx commented 7 years ago

@estaylorco Thank you for trying to help, but your use case isn't a good fit. I know there are several ways for the relevant use case(s) without this feature, I just don't like their additional complexity. I like the simplicity of navigate and navigateToRoute being able to pass all the relevant data to the receiving view model without having to go through the URL or some interim singleton. Especially since the router already implements the principle through the above functions and the currentInstruction object.

@PWKad Dealing with navigating the browser history is a concern I prefer to separate from the concern of passing data into the next route (especially since users of my apps generally expect persistent scroll positions in lists, zooms in maps etc when navigating back and forward). I'm aware of the settings property in route config, but using it means finding and updating a configuration object with run-time, instance data and it feels a bit messy.

@EisenbergEffect This feature isn't that important to me. I think there's a place for it and that it will be helpful in some cases, but if you're not in favour of it, by all means discard it. (It's easy enough for me to fork in for the projects where I want it.)

jwx commented 7 years ago

@EisenbergEffect @bryanrsmith Could you let me know whether this is something you want in the router or not? (I'm doing project clean up and if this was in I could skip a fork.) It's a small PR, 20 lines or so, but I don't want to waste anyone's time with something you don't want.

EisenbergEffect commented 7 years ago

I tend to think that the state could be stored in any class and retrieved by any class, using DI. It could even be done with the Optional.of DI resolver, if the state wasn't needed by the consumer. Why would it need to be part of the router?

jwx commented 7 years ago

@EisenbergEffect It can of course be done in other ways. For me, this (just like Aurelia) isn't about need; it's about usefulness and a prefered way of working. I use DI a lot, but I don't think it's a good fit for passing this type of information from one view to another when navigating. (Just like, for example, when I'm prefering .bind and not DI when passing values into child custom elements.)

It's only an additional optional parameter to navigate/navigateToRoute that would be added to the NavigationInstruction (in other words just a companion to the existing query params). I've created a PR for it (since I already had the branch) if you want to take a look.

And while I do think this is useful, it doesn't make or break anything for me so I'm fine with whatever you guys decide.

@bryanrsmith @davismj

davismj commented 6 years ago

@bigopon The general pattern for non-routed views is to use a <compose> element.

@jwx I want to say thank you for all your hard work recently. You've submitted many PRs and that's incredible.

Regarding this particular issue, I agree with @PWKad and @EisenbergEffect above, and I think @PWKad articulated my feelings exactly.

I guess I'm looking at it from the standpoint of maintenance and bug triage. As soon as that is added and someone hits the back button and their app breaks it's going to be a bug report.

Additionally, a number of community members have provided alternatives as well, and there hasn't been a strong call for this functionality. As such, I'm going to close.

An example I have is from a mobile app where selecting an item in a list of items navigates to /item/:id/ and passes the item data object from the list. The target view then initiates a fetch based on the id, but presents the item based on the passed object, if there is one, while waiting for the reply. (In some cases, it didn't even perform the fetch if the data was passed along.) It made a difference for the user experience.

I agree that this is a good design practice! I just think the router is the wrong place for it, I think this is logic that belongs in your service layer. So, say you have service.getAll() that gets objects { id, name }. Then you navigate to item/:id, and the active function calls service.get(id). Your service layer should then return the same object { id, name } immediately while making an async call to your API. When the call resolves, the service layer should enhance the very same object that was returned from this call with the additional parameters.

Is this more complicated? Yes. But I'd argue that this is a much more robust solution. There are two problems with the query string approach. First, it doesn't scale, and depends on passing not too much data on the query string. Your application may unexpectedly break when the query parameter grows suddenly and unexpectedly large. Second, it is a shortcut that will likely be abused by people who haven't learned the tools--and I see this all the time. One developer might use this feature carefully in a project, and another developer working on the same project will see this strategy and use it all the time until it breaks and the whole solution has to be refactored, inevitably opening up StackOverflow questions and GitHub issues along the way.