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 option to skip npmjs.org check in determineChannel #797

Closed
gregsadetsky opened this issue Apr 26, 2024 · 4 comments · Fixed by #812
Closed

add option to skip npmjs.org check in determineChannel #797

gregsadetsky opened this issue Apr 26, 2024 · 4 comments · Fixed by #812
Labels
help wanted Accepting PRs

Comments

@gregsadetsky
Copy link
Contributor

Is your feature request related to a problem? Please describe.

running mycli update ends up fetching information about a different package on npmjs.org which has the same URL identifier, and tries to identify the tag based on that unrelated package's version.

context: I'm working on a package called "disco". a similarly named package also exists on npm. as part of the update process, plugin-update fetches from registry.npmjs.org here in order to determine the tag value. in our case, the fetched tag is irrelevant since it's related to a completely different package.

Describe the solution you'd like

a few ideas:

  • a config to specify the default channel to use i.e. I could make mycli update default to the stable channel, and users could still pass a specific channel if they wanted. that way, no call to determineChannel would be necessary
  • a config to disable the npmjs check altogether? the check could be enabled by default for backwards compatibility
  • a config to specify the npmjs package name, and if absent, skip the npmjs check?

Describe alternatives you've considered

  • right now I could set config.npmRegistry to a nonsense value, ensuring that the try in determineChannel fails, but that's quite hacky
  • I could rename my package and set up a similarly named package on npmjs but that's pretty involved
@mdonnalley
Copy link
Contributor

mdonnalley commented Apr 26, 2024

a config to specify the default channel to use i.e. I could make mycli update default to the stable channel, and users could still pass a specific channel if they wanted. that way, no call to determineChannel would be necessary

This is my favorite of the ones you listed but I'm not sure how it would work for autoupdates. One of the use cases of the update command is to be able to set a channel and then plugin-update can autoupdate to the latest of that channel.

For instance, the user runs mycli update beta to update to the beta channel and then the autoupdate feature will periodically update the user to latest beta version. But in order for plugin-update to know what the latest beta is, it asks npm for the dist-tags of the package.

So if we have a config that tells plugin-update to always use the stable channel, then users would run mycli update beta and then some time later will be automatically updated back to stable - which would be surprising and frustrating to the user, since they specifically asked for the beta version.

The question you have to answer is, how does plugin-update know what the latest version of a specific channel is?

@gregsadetsky
Copy link
Contributor Author

thank you for your reply and this context!

I think that my "there should be a single default channel" solution is definitely too heavy handed and not practical.

here are some more thoughts after looking at the code a bit more:

  • the passed channel should be respected for sure -- this line does options.channel ?? determineChannel which is great -- use the given channel, otherwise try to determine the channel
  • determineChannel tries to determine the channel by first seeing if a channel file exists and defaulting to stable otherwise ((sorry to be explaining this by the way, you obviously know all of this -- I'm just re-stating this to sort of rubber duck solve this)) :-)
  • the issue is really that npmjs http call -- in the case of a package that is not distributed on npmjs, that call can only lead to two sorts of errors: a 404 (by happenstance) which will lead to that catch { return channel } or a successful read of an unrelated package which imho is the worst possible outcome -- no data is better than unrelated data.

so going back to my proposed solutions, I do feel that having the ability to disable the npmjs call would make the most sense? dontFetchChannelTagFromNpmJs or something less badly named? :-) and obviously false by default to ensure compatibility.

let me know what you think! and thanks again

@mdonnalley
Copy link
Contributor

so going back to my proposed solutions, I do feel that having the ability to disable the npmjs call would make the most sense? dontFetchChannelTagFromNpmJs or something less badly named? :-) and obviously false by default to ensure compatibility.

Yeah I think this is the way to go. Any interest in making the PR? Otherwise, I'm not sure that I have bandwidth to prioritize it.

Also, maybe call it disableNpmLookup?

@mdonnalley mdonnalley added the help wanted Accepting PRs label May 1, 2024
@gregsadetsky
Copy link
Contributor Author

hey, sorry for the delay -- yes I'd love to take a stab, thanks! I'll have something hopefully soon. cheers

gregsadetsky added a commit to gregsadetsky/plugin-update that referenced this issue May 4, 2024
mdonnalley pushed a commit that referenced this issue May 6, 2024
mdonnalley added a commit that referenced this issue May 6, 2024
* fix: dont check npm option - #797 (#807)

* chore: bump @ocilf/core

---------

Co-authored-by: Greg Sadetsky <lepetitg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Accepting PRs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants