search

shurcooL / Go-Package-Store

An app that displays updates for the Go packages in your GOPATH.
MIT License
900 stars 29 forks source link

Port to new Vecty API #84

Closed slimsag closed 6 years ago

slimsag commented 7 years ago

This change ports Go-Package-Store over to the new Vecty API, which recently had a breaking change.

Relies on PR https://github.com/gopherjs/vecty/pull/138 being merged first, which fixes a Vecty regression observed here.

No visual / functional changes are made, this just switches over to the new API. I ran out of time while writing this PR to properly make the commits very small / logical and reference exact README links. It's still best reviewed as individual commits, but the last commit in specific is not perfectly split apart into logical pieces. I won't have time to update this PR for probably a few more weeks, so if helpful please feel free to take / modify my changes here as you see fit :) I'll do my best to answer any questions here as I have time.

slimsag commented 7 years ago

Thanks a lot for making this PR Stephen! You didn't have to, I would've been fine updating it to use the new API myself too. But I appreciate it, thanks!

I know :) I wanted to! And I was curious how hard the upgrade path would be, and what unexpected things may be encountered. I was happy to do this because it ended up leading me to that regression we accidently introduced in Vecty (the PR mentioned in the description that is now merged).

dmitshur commented 6 years ago

@slimsag I believe I'm seeing a regression in rendering behavior. Look at these two frames, in sequence:

image

The frame above looks good. In the next frame, the only change that should happen is that "Checking for updates..." text should disappear. But in addition to that happening, the displayed packages become incorrect. The 4th package disappears, the first one becomes duplicated.

image

Tested using the version of Vecty roughly after https://github.com/gopherjs/vecty/pull/138 was merged, and using latest version. Same result.

dmitshur commented 6 years ago

Actually, let me confirm it's indeed a regression. It might not be. I tested with one pre-134 commit and I think it happens there too.

slimsag commented 6 years ago

Ouch, that's no good. I don't know if that is a regression, but if you can find steps to reproduce that please let me know and I'll do my best to investigate the cause as time allows.

dmitshur commented 6 years ago

Ok, it is a regression, but not with the new API. It happened before.

I did some archeology. I merged the Vecty branch and regenerated on April 13. I was using the latest Vecty commit at the time, which would be https://github.com/gopherjs/vecty/commit/a32ca065328c688881f95920bee2d78bd512c77b.

I checked out that commit locally and tested it out, the issue does not occur:

![image](https://user-images.githubusercontent.com/1924134/30617982-8d9124ca-9d66-11e7-9d1e-685666febda8.png)

I'll try to bisect to where it happened exactly. **Edit:** Done git bisect, reported results in gopherjs/vecty#149. In any case, that's a separate issue from the API change, so I won't let it stop me from merging this PR.
dmitshur commented 6 years ago

@slimsag I've pushed a 4th commit to this PR. It kinda reverts the code layout changes you've applied, but updates to use the new API.

Basically, my goal with this PR is to make minimal change to update to new API. I will consider code organization and style changes in followup changes - I don't want that to be a part of this one.

So, what I ended up doing is effectively replacing my previous use of vecty.List with []vecty.MarkupOrChild. That seems to be the equivalent thing of the new API. It works equally well in my testing.

dmitshur commented 6 years ago

Now that I understand that gopherjs/vecty#130 requires properties to be tagged with vecty:"prop" tags, I am curious, how come you didn't include that as part of this PR @slimsag? Did you forget or not realize it'd be needed? (/cc @pdf)