fix: use hasProxyTo to find correct uplink for tarballs

[Beanow]

Attempting to fix #1642

Reverting the fix commit will fail the test. However I'm not confident we can ignore the isUplinkValid check in all situations. The test is also a bit smelly, but can be used as reference.

Feel free to edit as needed.

[codecov]

Codecov Report

Merging #1644 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
+ Coverage    84.9%   84.92%   +0.02%     
==========================================
  Files          47       47              
  Lines        2477     2481       +4     
  Branches      571      572       +1     
==========================================
+ Hits         2103     2107       +4     
  Misses        370      370              
  Partials        4        4

Impacted Files	Coverage Δ
src/lib/utils.ts	93.98% <100%> (+0.13%)	⬆️

[juanpicado]

I like what I see 👍 but need to run more test, thank u for being so detailed.

[Beanow]

Let me know if there's anything you need from me for that 👍

[juanpicado]

👏 such a pleasure review this, thanks @Beanow , great stuff.

[juanpicado]

There is an interesting variant you should be aware

uplinks:
  cachednpm:
    url: https://registry.npmjs.org/
    cache: true
  npmjs:
    url: https://registry.npmjs.org/
    cache: false

packages:
  '**':
    access: $all
    publish:
    proxy: cachednpm npmjs

following your same example, if there is this scenario, where you can define multiple proxies for one package, the outcome of this would be no cache since both proxies will be true and the last one in the list remains, which would be npmjs.

If you try

  '**':
    access: $all
    publish:
    proxy: npmjs cachednpm

switching the proxy order, the outcome would be the same. This happens because the order of definition of uplinks matters.

Just keep this on mind.

[Beanow]

clap such a pleasure review this, thanks @Beanow , great stuff.

Glad to hear that 😄 thanks

There is an interesting variant you should be aware

That is interesting! It's probably best to write some of these scenarios out and agree on what they should do first.

For instance, both cache: false and cache: true from the same url.
Could be considered a config error, because there's no way we could both cache and not cache.
Or it could be considered something we can reduce. So it should cache.

And what about when the urls are different? cache: true for yarnpkg but cache:false for npmjs on the same package. What should that even do? 🤔

[juanpicado]

Agree, for now I guess the best is:

• Document the current behavior (website—> uplinks)
• Write more test to prove the current theory
• Open a discussion how we might handle it better

It is fue first time someone went so far with the conf :)

[lock]

🤖This thread has been automatically locked 🔒 since there has not been any recent activity after it was closed.
We lock tickets after 90 days with the idea to encourage you to open a ticket with new fresh data and to provide you better feedback 🤝and better visibility 👀.
If you consider, you can attach this ticket 📨 to the new one as a reference for better context.
Thanks for being a part of the Verdaccio community! 💘