gopherjs / gopherjs.github.io

GopherJS Playground
https://gopherjs.github.io/playground/
23 stars 12 forks source link

playground: Reload snippet on hash change. #17

Closed dmitshur closed 9 years ago

dmitshur commented 9 years ago

Due to the way browsers work, if the user manually edits the URL to change the hash to another snippet ID and presses enter, the first time it will cause a hash change event (typically scrolls page to the matching anchor, without reloading page). Second time the user presses enter, since the hash has not changed, it will instead reload the page, and in our case reload the snippet.

Therefore, before this PR, if the user manually edits the URL in browser to another snippet ID, first time it did not actually load it. After this PR, it will always try to load the snippet whose ID is in the URL box.

I think this leads to a better user experience as it eliminates a possible confusing behavior. It's also more efficient than reloading entire page, since that is not necessary.


Code-wise, I realize the XHR request code is starting to get out of hand. I tried to factor out the common code into a loadSnippet func or so, but it didn't quite work because of minor differences and need for local variable access.

I think a better way to go would be to add helpers to XHR package to make binary XHR requests more easily, since it's a pretty common requirement. Then, all XHR request code can go from:

req := xhr.NewRequest("GET", "http://"+snippetStoreHost+"/p/"+id)
req.ResponseType = xhr.ArrayBuffer
go func() {
    err := req.Send(nil)
    if err != nil || req.Status != 200 {
        scope.Apply(func() {
            scope.Set("output", []Line{Line{"type": "err", "content": `failed to load snippet "` + id + `"`}})
        })
        return
    }

    data := js.Global.Get("Uint8Array").New(req.Response).Interface().([]byte)
    scope.Apply(func() {
        scope.Set("code", string(data))
    })
}()

To simply:

data, err := xhr.SendBinary("GET", "http://"+snippetStoreHost+"/p/"+id, nil)
if err != nil || req.Status != 200 {
    scope.Apply(func() {
        scope.Set("output", []Line{Line{"type": "err", "content": `failed to load snippet "` + id + `"`}})
    })
    return
}

scope.Apply(func() {
    scope.Set("code", string(data))
})

Actually, that's not that much shorter. :/ But still cleaner I think.

dmitshur commented 9 years ago

Hmm, I just realized this PR is currently missing a history.PushState. If you change the hash to get to a different snippet, going "back" should take you to the previous snippet.

dmitshur commented 9 years ago

Never mind, that's already taken care of. The browser does it by default in this situation.

image

Pressing back/forward allows you to navigate to previous snippets.

So yeah, I think it's good to merge.

dmitshur commented 9 years ago

If there are no objections, I'll merge this tomorrow.

I think it's a clear improvement over current behavior, and I'm not seeing any issues so far.

dmitshur commented 9 years ago

Seems to work okay to me, not seeing any issues.

Note that the code needs updating from Str() to String(), but the PR worked okay because it contained already-compiled javascript.