-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix: add npm tag lookup to determineChannel correctly #563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with 1 suggestion
@@ -1,27 +0,0 @@ | |||
// import HTTP from 'http-call' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
src/update.ts
Outdated
return String(channel).trim() | ||
private async determineChannel(version:string|undefined): Promise<string> { | ||
try { | ||
const {body} = await HTTP.get(`https://registry.npmjs.org/${this.config.pjson.name}/`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proxy should be ok. That library looks to have implemented it
https://github.com/search?q=repo%3Aheroku%2Fhttp-call%20proxy&type=code
src/update.ts
Outdated
if (fs.existsSync(channelPath)) { | ||
const channel = await fs.readFile(channelPath, 'utf8') | ||
return String(channel).trim() | ||
private async determineChannel(version:string|undefined): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private async determineChannel(version:string|undefined): Promise<string> { | |
private async determineChannel(version?:string): Promise<string> { |
src/update.ts
Outdated
if (tag === 'latest-rc') return 'stable-rc' | ||
return tag | ||
} catch { | ||
return 'stable' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is stable
the right default? Would it be more accurate to fall back to what's in the channel file, like the original implementation did, then stable
as a last resort?
src/update.ts
Outdated
const channel = fs.existsSync(channelPath) ? (await fs.readFile(channelPath, 'utf8')).trim() : 'stable' | ||
|
||
try { | ||
const {body} = await HTTP.get<{'dist-tags':Record<string, string>}>(`https://registry.npmjs.org/${this.config.pjson.name}/`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad I got another look at this. I thought of something else.
There's a config and and env that oclif uses. see
https://github.com/oclif/core/blob/0c34a9d3d148f260384c0c8d3b7aa5ab974056ad/src/config/config.ts#L165
https://github.com/oclif/plugin-plugins/blob/ae60398dcf28fbbd65240c1979dceaadc86c35b4/src/yarn.ts#L61
https://github.com/oclif/oclif.github.io/blob/cb01783492cf177cc89be1717a39f5f396424fa0/docs/config.md#L34
I think this should support those.
QA notes: using sfv2(beta) via mac installer for updated without channel (went to v1) 📓 giving an invalid registry Couldn't find any way to break this, so 🚢 |
@W-12152081@
adds an npm registry lookup to find if the desired version number corresponds to a tag. defaults to
stable