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

feat: packages tracker ux logic #187

Closed
wants to merge 11 commits into from
Closed

feat: packages tracker ux logic #187

wants to merge 11 commits into from

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Mar 26, 2023

Fixes #170

How to test this version of the CLI?

To install it:

go install github.com/listendev/lstn/cmd/lstn@better-request-tracking

What happens?

A single request is made for every dependency instead of bulking them

The reason behind this is that by making a single request we can

  • avoid hitting the API timeout of 6 seconds (there is no dependency request that will take more than 200/300 ms round trip)
  • provide fine-grained progress tracking (this leads to better UX, the user can see things going on instead of waiting minutes for something to happen)
  • show specific errors when a dependency had an error (which is not one of our pre-defined Problems)

A progress tracker is shown

Now that one of the benefits of doing a single request is to be able to do fine-grained progress tracking, that's what happens!

Screenshots

here are some screenshots of how it looks like

lstn scan

A request is made for every version mentioned in the package.json
Screenshot 2023-03-27 at 01 58 59

lstn to react

This is the most basic use case, only the available versions are requested
Screenshot 2023-03-27 at 01 35 32

lstn to react *`

A request is made for every possible version on npm, the progress is shown like this. You will notice that the semver constraint is used to specify what we are processing
Screenshot 2023-03-27 at 01 44 21

lstn in

lstn in is not included in this. I did the code to actually include it but in is still very unstable for me at the moment until we do #132 . Once this gets merged I'll open an issue to do it.

TODO after merge:

  • once this gets merged, open an issue to support lstn in

@fntlnz fntlnz requested a review from leodido as a code owner March 26, 2023 23:59
@reviewpad reviewpad bot added large Pull request is large dependencies Something about our dependencies enhancement New feature or request needs-review Marks a pull request as waiting for review labels Mar 27, 2023
@fntlnz fntlnz force-pushed the better-request-tracking branch 3 times, most recently from 4a2a0dd to 829f7bc Compare March 27, 2023 00:31
@jadoonf
Copy link
Member

jadoonf commented Mar 28, 2023

LGTM

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for prototyping this @fntlnz!

As discussed privately, we need to revamp this and decide which issue we want to solve. Either:

  1. giving the CLI a progress indicator
  2. better handling of the deadline (timeout) errors in the case of "bulk" package requests

Info at #170 (comment)

@reviewpad reviewpad bot removed the needs-review Marks a pull request as waiting for review label Mar 28, 2023
@fntlnz
Copy link
Contributor Author

fntlnz commented Apr 5, 2023

closing this since this is not the approach we want to take now

@fntlnz fntlnz closed this Apr 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Something about our dependencies enhancement New feature or request large Pull request is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lstn scan not failing without any output
3 participants