linnarsson-lab / loom-viewer

Tool for sharing, browsing and visualizing single-cell data stored in the Loom file format
BSD 2-Clause "Simplified" License
35 stars 6 forks source link

proper URLs with react-router #6

Closed JobLeonard closed 8 years ago

JobLeonard commented 8 years ago

Mental note to look into this later:

https://github.com/reactjs/react-router

slinnarsson commented 8 years ago

Yes, it would be nice if you could link to a specific dataset from outside. As it stands now, the URL is always the root.

It would also be nice to be able to link to a particular view of a particular dataset (e.g. set up to visualize some specific thing). Not sure how easy it would be, because in principle you would need to encode the full state of the browser in the URL. Another possibility is to make it possible to save a view (e.g. as a JSON) under a name that can be given a URL. Just some ideas.

JobLeonard commented 8 years ago

After reading up on react-router, I think this is fairly simple to implement. I'm thinking of something along the lines of /dataset/<dataset url>/<view>/<settings>, where dataset is just to make it easier to distinguish it from /loom/ endpoints on the server side. dataset url and view should be human readable, settings could be base64 encoded to keep the length down.

Really, the only (expected) tricky bit will this:

Your server needs to deliver your app no matter what url comes in, because your app, in the browser, is manipulating the url. Our current server doesn't know how to handle the URL.

This is why I suggest putting a fixed /dataset/ before the <dataset url>/<view>/<settings>: it makes this easy to implement on the Python side: it just looks for /dataset/ and ignores everything that ocmes after it - the rest is only used client-side. The website then uses this under the hood to make ajax requeststo download in the requested dataset information, exactly the same way it is doing already.

To begin, we can first test & design the front-end side by navigating to loom.linnarssonlab.org and only clicking on links from there for now. Once that interface is stable we can update the python server.

JobLeonard commented 8 years ago

Ok, slight complication: currently all navigation is done via redux. That needs to be moved from there to make use of the react-router set-up; see also this advice on Stack Overflow):

Store is the source of truth for your data. This is fine. If you use React Router, let it be the source of truth for your URL state. You don’t have to keep everything in the store.

Could turn out to be quite a big rewrite, but the end-result should be a vast simplification of the navigation code. Since it will certainly break things until the migration is complete I've made a new branch for it.

JobLeonard commented 8 years ago

Writing down my thoughts on how to restructure the app.

Currently our redux store handles the following state:

The navigation state will be completely handled by the router, simplifying the code. The view state could be moved to the URL, and before I figured it woul be quite a bit of work with little pay-off. Given that it will greatly simplify the redux code I changed my mind on it though.

That leaves the fetching of datasets. There the code will also be simplified, since it no longer has to worry about setting up the data views.

Speaking of datasets, we currently don't check if a requested dataset has already been fetched before. Since it won't change (it's immutable data both server- and client-side), we might as well keep a fetched dataset in the redux store, and check for its presence. This essentially is manually caching datasets in javascript for the duration of a visit to loom; which is nice when the browser doesn't play nice with caching and while we haven't properly set up the caching on the server side yet (see #25). The potential downside I see to this is that if someone navigates through many datasets in one tab without closing it, it will end up using a lot of RAM to keep all the datasets in memory.

JobLeonard commented 8 years ago

So yesterday evening just before falling asleep I worked out a way to combine routing and redux and passing state to components in a way that feels elegant too (at least I think so). This of course means I forgot the fine details and have to start over again.. and that is why I'm writing all of it down, so I won't forget again and the structure of how it all fits together is worked out. It's long, but that's just because writing out programming stuff in human language is inherently verbose.

Here is the final URL once we have selected a dataset, a view and changed some view settings:

/dataset/:project/:transcriptome/:dataset/<view>/:viewsettings
\______/\_______________________________/\_____/\____________/
    |                   |                   |          |
    |                   |                   |      Parameters for a given view, passed
    |                   |                   |      as props to the view component.
    |                   |                   |
    |                   |             Heatmap, sparkline, etc.
    |                   |
    |            Encodes a request for a dataset, is compared against list
    |            of existing datasets first, cached after download.
    |            Displays dataset metadata and a list of possible views.
    |
  Fetches and displays a list of existing datasets.

The short version is that the URL hierarchy follows the component hierarchy, and URL parameters are interpreted at their own component level. Results are either stashed in the redux store or passed as props just like how React is normally set up:

Below I'm going to describe the desired behaviour in detail from left to right, while assuming we are deeplinking on a machine that has not visited the website before. Note that all of this is done by react-router; the server should just return a plain index.html for these URLs (if I understand the react-router api correctly this should "Just Work" but we'll see).

/dataset/

Will show a page that lets us select from the existing datasets.

Once the list grows in size, we might want to implement form with react-select fields that lets us filter sets by project, transcriptome, etc. This is low-priority: for now listing all existing datasets like is done currently will work perfectly fine. Given that implementing this form should not affect other parts of the system, there no pressure to implement it early in that sense either. Still, this shouldn't be too complicated: the server already provides a list of all existing datasets with fields containing metadata, all that needs to be done is list the fields in react-select components, and then filter the displayed list based on what is selected in those components.

Effect on current architecture

Little - it's basically just the existing dataset-view.js-component wrapped in react-router, with the current links replaced with react-router Link tags or a component wrapping them one way or the other; the existing closures will be moved to the linked components that handle those urls.

/dataset/:project/:transcriptome/:dataset/

First, {transcriptome}__{project}__{dataset} is deciphered from the url. Then an action is dispatched to compare against the list of existing datasets. The action handler for this will determine that the list is non-existent, so it will display a message "please wait, loading list of available datasets" and fetch the list. After that there are multiple possible results of the comparison to consider:

For now only the first one needs to be implemented. It should give a small page describing the metadata of the dataset, and provide links to all available views. The other options can just redirect back to the "choose a dataset" page with the message that a url to a non-existent dataset was passed.

Nice-to-have but probably not worth the hassle: if the filter-form mentioned earlier is implemented, a url like dataset/midbrain could pre-fill the project field with midbrain (and vice-versa: selecting a project would put its name in the URL). With that we could link and share all datasets for a given project, for example. However, it would probably take longer to implement than the number of times it will be used.

Effect on current architecture

/dataset/:project/:transcriptome/:dataset/<view>/ & /dataset/:project/:transcriptome/:dataset/<view>/:viewsettings

We do not need to store the entire state of the app to encode the view-settings; all we need is the differences from the default view settings. Assuming the worst-case scenario of loading a full URL on a machine that has never visited the Loom site:

Effect on current architecture

One net result of all this will be that the only two things that need to be stored in the redux store - for now - will be the list of available datasets and the downloaded datasets. Most other application state effectively "lives in the URL". redux and react-router can be combined, but I think that just complicates things for little gain in our application.

JobLeonard commented 8 years ago

Ok, so a summary of the progress made so far:

Server-side

Implementing the routing on the Flask side requires a catch-all URL. This appears simple enough, however, the naive solution proposed in that Stack Overflow page does not work because it messes up the serving of static files. Say we run locally:

Fixing this is probably a really stupid piece of configuration to set, but for now I configured flask to redirect all of the links mentioned to the root loom page. This defeats the purpose, but is still easier to debug than having to manually do this.

Client side routing

The landing page, upload and a list of datasets are all configured in react-router now. Everything except the specific view components (heatmap, sparkline, etc) has been heavily refactored; essentially all closures have been converted to components. I've also switched to react-bootstrap, which replaces all the juggling with nested <div className=x y z> tags with specific components like <Button> and <NavBar>. The whole thing has become a lot more readable as a result.

Furthermore, the top-level connect that we used is gone now. Instead I'm planning to connect each view separately (which is what the redux documentation suggests; it's supposedly faster too); dataset-view is already using this method. It took a lot of trial and error to get all bits and pieces to work together, but I think I figured it out now.

Next step is refactoring the views (most of which are actually fairly untouched since you created them). There's a lot of view configuration and state going on in there so I'm writing the whole thing out on paper first to figure out what is going on and how they process all the provided data.

JobLeonard commented 8 years ago

Heads-up: expect a few days of no commits as I'm figuring out how everything is wired together in dataState, heatmapState, landscapeState, genescapeState and viewState and how this should be rewired with react-router. Will write down my thoughts here as they get worked out.

slinnarsson commented 8 years ago

I think I fixed the catch-all URL issue. Here's what I did:

  1. Made endpoints on the server to catch all / and /dataset/... endpoints and always return index.html (not redirect to it). This was based on what you wrote above (where everything is attached to the /dataset endpoint). I removed the /view endpoint which was in the server-side routes - let me know if you need it.
  2. Changed the URL in index.html from static/css/bundle.min.css to /static/css/bundle.min.css.
  3. Changed the URL in webpack.config.js from static/js/bundle to /static/js/bundle.

Now, if I load http://localhost:8003/view I get the same view as if I load http://localhost:8003/ and the javascript and CSS both load correctly.

JobLeonard commented 8 years ago

Since we're stripping uploading from the client side (see #8 ), we might as well set the dataset list as the landing page.

Also, I suggest removing the navbar altogether and simply add a "back" button to the view settings. It makes the whole interface extremely minimal, but it saves screen space and code.

slinnarsson commented 8 years ago

But if there's no Navbar, how do you switch between views? Do you then need to go back to the dataset list to get from heatmap to scatterplot? That's a bit clunky. It's important to be able to switch quickly and preferrably with a single click.

One possibility is to replace the navbar with a row of buttons/icons at the top of the sidebar (i.e. a Bootstrap "Button group"). There would be room for only 4-5 icons max, but maybe that's enough. They need to be big enough to be easy to click (and tap on a tablet). Lesser-used views could be grouped in a dropdown button at the end of the toolbar. Doing it this way would save screen space and make the plots larger, which is always good. The disadvantage is that it would be hard to fit descriptive words in the toolbar. "Heatmap", "Cells", "Genes", "Bars" already takes a lot of space.

The landing page would be the list of datasets. Click one and it takes you into the default view for that dataset (e.g. a scatterplot). The sidebar shows options for that view, plus a row of icons at the top to switch to a different view. One of the icons is the "house" for "go home", which takes you back to the list of datasets and lets you pick a different one.

If there are no datasets, the landing page instead shows a quick "Getting started" instruction in a Bootstrap Jumbotron. For example, it describes how to obtain datasets, where to put them, and where to get more detailed help. Maybe even a button to automatically download an example dataset to get started?

JobLeonard commented 8 years ago

That was more or less what I had in mind: add the views to the settings tab, perhaps as a drop-down menu.

The current settings tabs has room for improvement in more areas, actually. I'll open a separate issue for it.

JobLeonard commented 8 years ago

Ok, so for now I'm leaving in the navbar and made it well-behaved with react-router, but overall I'd like to fuse it into the sidebar-menu.

I think we're ready to merge, but be warned that some things will be broken at first and layouts will look terrible because #28, #31 and #35 are still in progress (also relates to #11).

JobLeonard commented 8 years ago

Closing this issue since the main goal of it is done with the last merge; further tweaks go into new issues.