Render HTML on frontend, add "Update All" button. #71
Conversation
|
This currently relies on support for frontend HTTP streaming via fetch API, in order to display updates as they come in. I know latest stable Chrome supports that, but the latest stable Safari does not. On browsers that don't support it, all updates will show up at once at the end. See http://caniuse.com/#feat=fetch. It's possible to rewrite it to avoid relying on HTTP streaming, so I'll need to think about whether that's a blocker for this going in. |
|
Safari 10.1 includes support for Fetch API (it's actually the first feature listed), see its release notes. It should ship with next macOS 10.12.4, which is going to be happen relatively soon. |
|
Apple has officially released macOS 10.12.4 today. It ships with Safari 10.1, which supports Fetch API. So, this PR is no longer blocked on that. |
Smaller diff in future commits.
This is a large change that primarily moves HTML rendering and display logic from backend to frontend (#67). Previously, the HTML for displaying updates was rendered on backend and streamed to browser. This worked surprisingly well and got me far, but in order to be able to have more fine grained control over frontend details, it was no longer viable to keep doing that. Now, the HTML is fully rendered on frontend, and most of the logic resides on the frontend. The backend provides services to the frontend. See issue #67 for full rationale why this is desired. Implement a long-standing feature request of having an "Update All" button (#6). This is both made possible and easy thanks to the frontend HTML rendering. This change partially helps #63, but also enables potential future changes to help it even more. In general, the move to frontend HTML rendering will help potentially achieve some of enhancements described in #8. Close #66 by removing the popup altogether. It wasn't well implemented, so it's better to remove. In the future, a better replacement implementation of the notification (without the modal popup) can be considered. Resolves #67. Closes #66. Helps #63. Helps #8. Resolves #6.
![gopherci[bot]](https://avatars.githubusercontent.com/in/889?s=60&v=4){kind=small}
cmd/Go-Package-Store/update.go
Outdated
| "github.com/shurcooL/httperror" | ||
| ) | ||
|
|
||
| func NewUpdateWorker(updater gps.Updater) updateWorker { |
There was a problem hiding this comment.
golint: exported function NewUpdateWorker should have comment or be unexported
cmd/Go-Package-Store/update.go
Outdated
| "github.com/shurcooL/httperror" | ||
| ) | ||
|
|
||
| func NewUpdateWorker(updater gps.Updater) updateWorker { |
There was a problem hiding this comment.
golint: exported func NewUpdateWorker returns unexported type main.updateWorker, which can be annoying to use
component/header.go
Outdated
| "golang.org/x/net/html/atom" | ||
| ) | ||
|
|
||
| type Header struct{} |
There was a problem hiding this comment.
golint: exported type Header should have comment or be unexported
component/header.go
Outdated
|
|
||
| type Header struct{} | ||
|
|
||
| func (Header) Render() []*html.Node { |
There was a problem hiding this comment.
golint: exported method Header.Render should have comment or be unexported
component/header.go
Outdated
| CheckingUpdates bool | ||
| } | ||
|
|
||
| func (u UpdatesHeader) Render() []*html.Node { |
There was a problem hiding this comment.
golint: exported method UpdatesHeader.Render should have comment or be unexported
component/presentation.go
Outdated
| @@ -22,20 +21,28 @@ type RepoPresentation struct { | |||
| Changes []Change | |||
| Error string | |||
|
|
|||
| Updated bool | |||
| UpdateState UpdateState | |||
|
|
|||
| // TODO: Find a place for this. | |||
| UpdateSupported bool | |||
There was a problem hiding this comment.
golint: comment on exported type RepoPresentation should be of the form "RepoPresentation ..." (with optional leading article)
component/presentation.go
Outdated
| Updating | ||
| Updated | ||
| ) | ||
|
|
||
| func (p RepoPresentation) Render() []*html.Node { | ||
| // TODO: Make this much nicer. | ||
| /* | ||
| <div class="list-entry go-package-update" id="{{.Repo.Root}}" style="position: relative;"> | ||
| <div class="list-entry-header"> | ||
| {{.importPathPattern()}} | ||
|
|
There was a problem hiding this comment.
golint: exported type UpdateState should have comment or be unexported
component/presentation.go
Outdated
| func (p RepoPresentation) Render() []*html.Node { | ||
| // TODO: Make this much nicer. | ||
| /* | ||
| <div class="list-entry go-package-update" id="{{.Repo.Root}}" style="position: relative;"> | ||
| <div class="list-entry-header"> | ||
| {{.importPathPattern()}} | ||
|
|
||
| {{if (not .Updated)}}{{.updateButton()}}{{end}} | ||
| <div style="float: right;">{{.updateState()}}</div> | ||
| </div> |
There was a problem hiding this comment.
golint: exported const Available should have comment (or a comment on this block) or be unexported
component/presentation.go
Outdated
| @@ -56,13 +63,13 @@ func (p RepoPresentation) Render() []*html.Node { | |||
| FirstChild: p.importPathPattern(), | |||
| }, | |||
There was a problem hiding this comment.
golint: exported method RepoPresentation.Render should have comment or be unexported
The goal of this change is to be optimize frontend rendering performance when there's a large number of updates available. This is done by using Vecty (https://github.com/gopherjs/vecty) to be able to make incremental changes to the current page, rather than having to re-render the entire page from scratch for each update. Another technique that's used is to create a scheduler. Its job is to batch many update operations that occur rapidly one after another, apply them all, and perform a re-render just once. This helps because rendering the entire page once is faster than doing as many renders as there are updates. There is still many WIP and development code left commented out, it will be used and cleaned up over time. Some of that is waiting on a resolution to hexops/vecty#92.
go generate ./...
|
I've merged this now. There's still room for optimization, improvement, and cleaning up code. But the current version is stable and I can't find any blocking issues in it without wider testing. I'm waiting on a resolution to hexops/vecty#92 to be able to experiment with There will be ongoing incremental cleanup and improvement done after this is merged (especially if there are issues reported). I've added a screenshot of the final version to the PR description. |
This is a large change that primarily moves HTML rendering and display logic from backend to frontend (#67).
Previously, the HTML for displaying updates was rendered on backend and streamed to browser. This worked surprisingly well and got me far, but in order to be able to have more fine grained control over frontend details, it was no longer viable to keep doing that. Now, the HTML is fully rendered on frontend, and most of the logic resides on the frontend. The backend provides services to the frontend. See issue #67 for full rationale why this is desired.
Implement a long-standing feature request of having an "Update All" button (#6). This is both made possible and easy thanks to the frontend HTML rendering.
This change partially helps #63, but also enables potential future changes to help it even more.
In general, the move to frontend HTML rendering will help potentially achieve some of enhancements described in #8.
Close #66 by removing the popup altogether. It wasn't well implemented, so it's better to remove. In the future, a better replacement implementation of the notification (without the modal popup) can be considered.
Screenshot