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

Addressing some feedback from new yarn source #233

Merged
merged 6 commits into from
Jan 2, 2020
Merged

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Dec 30, 2019

👋 @sergey-alekseev @krzysztof-pawlik-gat thanks for taking a look at the PR for the new yarn source!

I'd appreciate if you could also take a look at this PR and let me know if I'm addressing all of your comments and feedback 🙇

  1. protecting against extra json output from yarn list and yarn info by searching for the tree and inspect types, respectively.
  2. moving JSON.parse outside the parallel handling from Parallel.map
    • I think general lack of thread-safety might have been the cause of @sergey-alekseev's seen error
    • the reason to parallelize the calls is that yarn info is slow. that call is still parallelized 👍
  3. nit cleanup on the yarn fixture

@krzysztof-pawlik-gat
Copy link

Thank you @jonabc, this looks good, I found one more issue but it's in review comments.

@sergey-alekseev
Copy link
Contributor

Seems like the error which I previously reported is gone 👍

I tested the yarn source locally again and it seems to be working well. In comparison with the bundler source it takes quite a bit of time. See below.

When I run licensed status it immediately progresses with bundler dependencies and then stuck for some time without any progress with yarn dependencies. Finally it completes and prints everything at once. In my case:

# with yarn source enabled
$ time licensed status
...
1584 dependencies checked, 198 errors found.

real	3m50.166s
user	9m12.936s
sys	1m30.953s

# with yarn source disabled
$ time licensed status
...
190 dependencies checked, 0 errors found.

real	0m0.972s
user	0m0.709s
sys	0m0.252s

licensed cache takes a lot of time too even when dependencies are already cached. In my case:

# with yarn source enabled
$ time licensed cache
...
  * 1394 yarn dependencies

real	3m41.637s
user	9m30.477s
sys	1m33.880s

# with yarn source disabled
$ time licensed status
...
  * 190 bundler dependencies

real	0m1.304s
user	0m0.897s
sys	0m0.370s

Thanks for your work @jonabc! Happy New Year 😃

@jonabc
Copy link
Contributor Author

jonabc commented Jan 1, 2020

Yea the yarn source is really slow compared to bundler due to the extra shell execution of yarn info on each yarn dependency. In your example that looks like around 1400 total calls to yarn info. That's compared to npm which gets all of its info from a single npm list, or bundler which primarily uses the bundler API to get data, and so are much faster.

The extra call is used to grab the description and homepage. The description isn't as important, but giving proper attribution for the homepage might be? Not sure TBH

@sergey-alekseev
Copy link
Contributor

sergey-alekseev commented Jan 1, 2020

Thanks for the info @jonabc. I haven't dug into details and code, but my first thought is "can we avoid calling yarn info for licensed status and for already cached dependencies during licensed cache?"

@jonabc
Copy link
Contributor Author

jonabc commented Jan 1, 2020

@sergey-alekseev good idea! That would require deeper changes than would make sense to include in this PR though 😞.

Can you open an issue? I have a few ideas on how I would structure the change that I can add to the new issue, but if you wanted to give that change a shot and open a PR I would gladly review it 😄

Copy link

@krzysztof-pawlik-gat krzysztof-pawlik-gat left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@sergey-alekseev
Copy link
Contributor

@jonabc I opened an issue regarding skipping yarn info as you requested – #235.

@jonabc jonabc merged commit 63f056a into master Jan 2, 2020
@jonabc jonabc deleted the yarn-feedback branch January 2, 2020 14:13
This was referenced Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants