History #135

Merged
merged 5 commits into from Feb 26, 2012

Projects

None yet

2 participants

@nrn
Collaborator

Getting my hands dirty with some client side stuff.

The first commit is an easy one, just made external links go to a new tab by default. Living with it for a while it seems to be the desired behavior for single page apps like this. Also adds safety for trailing slash in urls on client side.

The second commit is a big one. I started out trying to get hidden state out of the history. It seemed to be odd behavior that the forward and back buttons would scroll the page, and it was very hard to reason about how that information was being passed around with the history changes. So I got rid of all the stuff that was being passed around in the history state, instead relying on the dom as truth.

I also removed history.js. This change has the disadvantage of making a certain class of oldish browsers not update their urls, however it has the advantage of allowing an even broader range of browsers to navigate around the page correctly. History.js doesn't seem to be maintained anymore, and the direct history api seems to really be what we want to use in the future. I think the trade off for a more graceful degradation instead of pollyfilling just for mid-old browsers is a good one.

So I hope this achieves my initial goal of making this part of the code easier to reason about, it seems to from my perspective, but that may just be because I wrote it :)

@WardCunningham

I'm having some trouble with this commit. Here's the test conditions:

I'm in Chrome
I'm running against the Sinatra server
I type: localhost:1111 in the location bar
I get the OpenID form but no Welcome Visitors page

I also get this javascript error:

client.js:253  Uncaught TypeError: Cannot read property 'left' of null

When I try the same thing in Firefox there is no error. Nor is there an error when I run the integration tests. Of course these run against Firefox, not Chrome.

I'll wait for you to have a look at this before I pull.

@WardCunningham

Pushed a few wiki.log in branch history to see if my buddy Alex and I can figure out what's going on. We think it has something to do with how the Sinatra server sets up initial divs. At least we are suspicious of the code around here

This is trace output showing variations between different server/browser combinations:

 Sinatra/Chrome     ["amost createPage", Array[0], Array[1], Array[1]] *** FAIL ***
 Express/Chrome     ["amost createPage", Array[1], Array[1], Array[1]]

 Sinatra/Firefox    ["amost createPage", [], [""], ["welcome-visitors"]]
 Express/Firefox    ["amost createPage", ["welcome-visitors"], ["view"], ["welcome-visitors"]]

The Array[1] elements reported by Chrome are exactly those reported as quoted strings in the Firebug console.

Express emits a status 302 redirect to the welcome-visitors url while Sinatra relies on the client code reading welcome-visitors out of the html returned on the HTTP / request.

@nrn
Collaborator

Well, that is some very odd behavior. Express is doing the same thing as sinatra and appending the divs before it sends out the html. I'm off to bed but i'll dig into it as soon as I can.

@nrn
Collaborator

I can't seem to reproduce this. Running the ruby version from your history branch, and chromium from debian testing repo everything looks as it should. Any ideas on what I could be missing?

@WardCunningham

Could it be a Mac OSX problem?

What problem does the test for empty string solve in that code?

@nrn
Collaborator

That was a safety to guard against a trailing / in the url which would cause an empty page to get created on static server implementations.

In your logging does the wiki.log 'createPage' ever get called on line 531? Cause if so that is a bug in any dynamic server implementation (github 404 magic uses it, and it could be used with other static servers). Every page in the url should be in the dom when the page first loads so I think we're chasing down the wrong section of code here. Those logs are to be as expected if you are sending the initial contact with the pages in the html but not the url.

@WardCunningham

I'll did a little deeper tonight. I seem to have the only failing setup.

@nrn
Collaborator

Guessing because I don't have the broken setup to prove it, but I think the problem is you need a setState() line around 526 for the ruby server in some instances. But in testing this i've found another problem with the fetch stuff using the new history stuff. Hopefully get another commit together tomorrow fixing this stuff.

@WardCunningham
@nrn
Collaborator

Glad to.

This last commit makes calls to fetch non destructive to the global wiki.fetchContext, as well as making setActive and setState more explicit, scrollTo is also only called from setActive now.

The long and short of this branch is that i'm trying to make it easier to figure out what is going on as we navigate through the browser history as well as making those interactions more robust. It should be getting pretty close to pull worthy, i'm hoping this last commit also fixed the error you were seeing with osx/chrome/sinatra.

@nrn
Collaborator

If this is working on your setup I think this branch is ready to ship. This last commit just changes setState to setUrl (to be a little clearer), and moves the popstate handler down with the rest of the event handlers. It passes all the tests and seems to work well in use on all the setups I've got available at the moment, as well as poking around a bit with various setups on http://browserling.com/

Let me know if there is anything else that needs to change in this one.

@WardCunningham WardCunningham merged commit c5b67dd into WardCunningham:master Feb 26, 2012
@WardCunningham

Thanks for taking the time to think this through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment