Skip to content
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

Add `add-tag-to-commits` feature #2026

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dotconnor
Copy link
Contributor

commented May 12, 2019

Note: For some reason, the API returns empty data on some tags

Closes #807

@dotconnor dotconnor changed the title feat: add tags to commit list Add `add-tag-to-commits` feature May 12, 2019

@bfred-it
Copy link
Collaborator

left a comment

}`);
let tags: Tag[] = repository.refs.edges.map((edge: any) => edge.node.target).filter((tag: Tag) => tag.name && tag.commitResourcePath).map((tag: Tag) => ({...tag, commit: tag.commitResourcePath.split('/')[4]}));
if (repository.refs.pageInfo.hasNextPage) {
tags = tags.concat(await getTags(repository.refs.pageInfo.endCursor));

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

This is doing exactly something that I didn’t want to do: fetch multiple pages of tags on every Commits page 😕

This comment has been minimized.

Copy link
@dotconnor

dotconnor May 13, 2019

Author Contributor

From what I've seen 100 tags (1 request) is enough for most repos, the only encounter I had with more than that was facebook/react

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

It's true, but also there are repos like https://github.com/nodejs/node

Are 6 sequential requests acceptable? Are these heavy? How difficult would it be to only query the next page if no matching tags are found on the current one?

This comment has been minimized.

Copy link
@dotconnor

dotconnor May 13, 2019

Author Contributor

nodejs/node doesn't tag their releases on master so even if you did, you would still pull all 6 requests each time.

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus May 24, 2019

Owner

fetch multiple pages of tags on every Commits page

I assume we can cache the tags? At least the older ones.

Are these heavy?

@dotconnor ⬆️

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 24, 2019

Collaborator

I assume we can cache the tags? At least the older ones.

Cache where? Our cache is rather small and if we're gonna store them in a regular browser.storage.local we also need a cleaning mechanism so it doesn't stay there forever.

Are these heavy?

I think the definition of heavy is "what's the API cost?" and "how long does it take for an answer?"

If the tags appear 5 seconds after dom ready then I probably won't see them or don't know if they're still loading.

This comment has been minimized.

Copy link
@dotconnor

dotconnor May 24, 2019

Author Contributor

I assume we can cache the tags? At least the older ones.

Sure we could cache the tags, but you would still have to make the API calls to validate that the remaining commits don't have tags. So caching based on the commit (key) and its associated tags (value), and only make a network request if a commit isn't found in the cache, would work best.

Cache where? Our cache is rather small and if we're gonna store them in a regular browser.storage.local we also need a cleaning mechanism so it doesn't stay there forever.

I think a simple LRU implementation on top of browser.storage.local with lazy saving/cleaning should suffice.

Are these heavy?

Speed? Really depends on Github, from my testing its a little all over the place from ~200ms to sometimes a full second.

Size? A full page of tags (100) is about 4KB

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 25, 2019

Collaborator

I think a simple LRU implementation on top of browser.storage.local with lazy saving/cleaning should suffice.

If you want to implement that it would be great, you can keep it as a separate PR and replace the existing cookie cache.

dotconnor added some commits May 13, 2019

@dotconnor

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Note: For some reason, the API returns empty data on some tags

The API only returns the Tag type on annotated tags and not lightweight ones for example: 123425d, and from what I've can tell you can't retrieve the tag name for the Github API.

dotconnor added some commits May 24, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2019

The API only returns the Tag type on annotated tags and not lightweight ones

This works: https://stackoverflow.com/a/53923870/288906

Screenshot 2019-05-25 at 16 26 03

Compared to the query in this PR:

Screenshot 2019-05-25 at 16 26 11

dotconnor added some commits May 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.