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

Port to new Vecty API #84

Merged
merged 4 commits into from Sep 20, 2017

Conversation

Projects
None yet
2 participants
@slimsag
Contributor

slimsag commented Sep 4, 2017

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

Relies on PR gopherjs/vecty#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.

@dmitshur

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!

Show outdated Hide outdated component/presentation.go
@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Sep 5, 2017

Contributor

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).

Contributor

slimsag commented Sep 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 19, 2017

Member

@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 gopherjs/vecty#138 was merged, and using latest version. Same result.

Member

dmitshur commented Sep 19, 2017

@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 gopherjs/vecty#138 was merged, and using latest version. Same result.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 19, 2017

Member

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.

Member

dmitshur commented Sep 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Sep 19, 2017

Contributor

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.

Contributor

slimsag commented Sep 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 19, 2017

Member

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 gopherjs/vecty@a32ca06.

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

image


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.

Member

dmitshur commented Sep 19, 2017

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 gopherjs/vecty@a32ca06.

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

image


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.

Return to previous code layout.
Replace the previous uses of vecty.List and its expansion with
[]vecty.MarkupOrChild and its expansion. This allows keeping the code
layout as before.

Keep container elements separate from the content that they wrap, as
before. I find it more readable and easier to maintain this way (at
least at this time).
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 19, 2017

Member

@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.

Member

dmitshur commented Sep 19, 2017

@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

LGTM. Thank you!

(I won't be able to actually regenerate and use the new code until we resolve gopherjs/vecty#149.)

@dmitshur dmitshur merged commit e7b1bc5 into shurcooL:master Sep 20, 2017

2 checks passed

ci/gopherci/pr Found no issues \ʕ◔ϖ◔ʔ/
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 10, 2017

Member

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)

Member

dmitshur commented Oct 10, 2017

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)

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