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

Router and browser reload #31

Open
johnsusi opened this issue Nov 10, 2015 · 8 comments
Open

Router and browser reload #31

johnsusi opened this issue Nov 10, 2015 · 8 comments

Comments

@johnsusi
Copy link
Contributor

I had some issues when trying to reload a page after using Router::navigate_to or when clicking a Link element.

To solve this I created a custom Router like this

class Router < Clearwater::Router

  def initialize
    super
  end

  def current_path
    path = location[:hash]
    return (path && path.start_with?('#')) ? path[1..-1] : path
  end

  def navigate_to path
    push_state "##{path}"
    set_outlets
    render_application
  end
end

So now the router keeps track of the view using fragments and location[:pathname] stays fixed.

Is there some 'official' way of doing this?

@jgaskins
Copy link
Member

Unfortunately, I haven't added support for fragment-based history at all. It's something I've been thinking about for a while, but I haven't gotten around to it. TBH, since nobody had asked, I wasn't sure it was a problem. :-) I can work on it a bit this weekend unless you'd like to send a PR before then.

The thing is, we have to somehow get Router and Link to work the same when it comes to navigation, so if Router supports fragment-based routing, Link has to support fragment-based navigation. This is the tricky part. Currently, Link's onclick doesn't go through Router#navigate_to because you don't know which instance of Router is in use by the app (you can have multiple Clearwater apps on the same page, with separate routers). I was thinking of making navigate_to a class method to fix that (since it's only changing global state, it should be fine to have it globally accessible), but I haven't checked whether that has other consequences.

Honestly, the router is the thing I'm least comfortable with code-wise. Once the tests went green while I was writing it, I basically committed it and backed away slowly. :-) I've done very little with it since then except to simplify a few things.

@johnsusi
Copy link
Contributor Author

I think the reason we had problems with this is that we are running in electron (atom-shell) as a single-page application so we never hit a server on reload. I guess it's not a problem if you can map everything below a base url to the same server-side generator.

Making navigate_to static (I'm not a ruby programmer ;-) ) sounds reasonable but perhaps we also need a flag to enable fragment based routing since it is probably not what you want in a classic client/server scenario.

For Link this is how I do it today to get it working Link.new({ href: "#some_view" }). But it would be better if that was transparent to avoid bugs.

Feel free to close this since it was more of a question that an issue :)

@jgaskins
Copy link
Member

Ahhh, cool. I wonder if we should just add a fragment-based router or make the history object inside the router (currently using window.history directly) automatically switch to a fragment-based approach if history.pushState isn't detected.

The automatic switching between fragment and pushState is something I've mulled over in my head a lot but haven't really codified yet because I haven't been sure it'd be worth it. I'd been mostly thinking that it was just IE < 10 that would need to worry about that. Not being able to use it in Electron wasn't something I'd thought of at all. Does Link work and it's just the router that's not routing properly?

I'm totally gonna keep this issue open until we can solve this. It may just be a question, but if people want to build Electron apps, I think this is all valid conversation to have. :-)

@johnsusi
Copy link
Contributor Author

Actually most things worked, but we ran into some issues after using navigate_to "/some/route". After that we could not load images anymore and it wasn't obvious (to us) why. After reading through the code and finding pushState things became clear :) (the leading / caused the documentURI to be cleared)

I think my major issue with the router is that it is an object instance on my app, but uses a global for state. That can lead to all kinds of weird behaviour that is hard to debug. Making navigate_to static at least makes that behaviour explicit.

A better approach for our application would be that Link uses the current rendering applications router instead of using href. (Or if Link is intended as a 'fixed' , introduce another component that works that way). The router would handle whatever needs to be done, be it pushState or just updating the current app model. The important part is that it is very explicit where you would handle that logic.

As a side note, when i implemented something similar in React, I passed down a route(path, ...args) prop to the components instead that they could invoke in response to events.

@jgaskins
Copy link
Member

A better approach for our application would be that Link uses the current rendering applications router instead of using href. (Or if Link is intended as a 'fixed' , introduce another component that works that way). The router would handle whatever needs to be done, be it pushState or just updating the current app model. The important part is that it is very explicit where you would handle that logic.

I think Link solves exactly the problem we need it to solve on the web side, so a new component might be in order. I've never used Electron, but I'm really interested in this, can you post a sample Clearwater+Electron app I can play around with this weekend?

@jgaskins
Copy link
Member

jgaskins commented Jan 2, 2016

@johnsusi Do you have a link to a minimal/hello-world Clearwater+Electron app I could use to get started on trying to ensure the two work well together? :-)

In addition to wanting to make sure routing works well for it, since I've removed the dependency on opal-browser to try to reduce payload size for browser apps I want to make sure the new browser abstraction works well with Electron, too.

@jgaskins jgaskins mentioned this issue Jan 10, 2016
7 tasks
@johnsusi
Copy link
Contributor Author

@jgaskins Sorry for the late reply. I must have missed the notification.

I put up a simple project at https://github.com/johnsusi/electron-clearwater

There are some issues with it, for instance virtual-dom.js does not appear to be loading correctly but it's a start. I intent to add scripts for packaging native apps.

@jgaskins
Copy link
Member

Ah, yeah, I remember when I played with getting Electron to work with Clearwater (after you opened this issue), it seemed to have 2 JS environments: v8 running inside the app window and Node running on the back end.

I have no idea if this is accurate, I'm going off of stale memories. :-)

I remember I had to get the Clearwater code running in the app window side, so basically I was loading its file as a <script> tag inside the index.html file. I couldn't get it working otherwise. :-\ That was my one and only experience with Electron, though, so you might be able to do better with it.

I'm really glad you're experimenting with this. Let me know if there's anything I can help with! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants