Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions packages/core/src/shared/lsp/lspResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import AdmZip from 'adm-zip'
import { TargetContent, logger, LspResult, LspVersion, Manifest } from './types'
import { getApplicationSupportFolder } from '../vscode/env'
import { createHash } from '../crypto'
import request from '../request'
import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher'

export class LanguageServerResolver {
constructor(
Expand Down Expand Up @@ -165,9 +165,8 @@ export class LanguageServerResolver {
}

const downloadTasks = contents.map(async (content) => {
// TODO This should be using the retryable http library but it doesn't seem to support zips right now
const res = await request.fetch('GET', content.url).response
if (!res.ok || !res.body) {
const res = await new HttpResourceFetcher(content.url, { showUrl: true }).get()
if (!res || !res.ok || !res.body) {
return false
}

Expand Down
13 changes: 5 additions & 8 deletions packages/core/src/shared/lsp/manifestResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

import { getLogger } from '../logger/logger'
import { ToolkitError } from '../errors'
import { RetryableResourceFetcher } from '../resourcefetcher/httpResourceFetcher'
import { Timeout } from '../utilities/timeoutUtils'
import globals from '../extensionGlobals'
import { Manifest } from './types'
import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher'

const logger = getLogger('lsp')

Expand Down Expand Up @@ -40,14 +40,11 @@ export class ManifestResolver {
}

private async fetchRemoteManifest(): Promise<Manifest> {
const resourceFetcher = new RetryableResourceFetcher({
resource: this.manifestURL,
params: {
timeout: new Timeout(manifestTimeoutMs),
},
})
const resp = await new HttpResourceFetcher(this.manifestURL, {
showUrl: true,
timeout: new Timeout(manifestTimeoutMs),
}).getNewETagContent(this.getEtag())

const resp = await resourceFetcher.getNewETagContent(this.getEtag())
if (!resp.content) {
throw new ToolkitError('New content was not downloaded; fallback to the locally stored manifest')
}
Expand Down
45 changes: 0 additions & 45 deletions packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,51 +134,6 @@ export class HttpResourceFetcher implements ResourceFetcher<Response> {
}
}

export class RetryableResourceFetcher extends HttpResourceFetcher {
Copy link
Contributor

@justinmk3 justinmk3 Jan 23, 2025

Choose a reason for hiding this comment

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

Nice that we can eliminate this! Generally better to have composable concepts, or at least flexible classes, instead of specific subclasses that implement particular compositions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was one of the motivations for doing all this httpResourceFetcher stuff

private readonly requestTimeoutMs: number
private readonly retryIntervalMs: number
private readonly resource: string

constructor({
resource,
params: { retryNumber = 5, retryIntervalMs = 3000, showUrl = true, timeout = new Timeout(5000) },
}: {
resource: string
params: {
retryNumber?: number
retryIntervalMs?: number
showUrl?: boolean
timeout?: Timeout
}
}) {
super(resource, {
showUrl,
timeout,
})
this.requestTimeoutMs = retryNumber * retryIntervalMs
this.retryIntervalMs = retryIntervalMs
this.resource = resource
}

fetch(versionTag?: string) {
return waitUntil(
async () => {
try {
return await this.getNewETagContent(versionTag)
} catch (err) {
getLogger('lsp').error('Failed to fetch at endpoint: %s, err: %s', this.resource, err)
throw err
}
},
{
timeout: this.requestTimeoutMs,
interval: this.retryIntervalMs,
retryOnFail: true,
}
)
}
}

/**
* Retrieves JSON property value from a remote resource
* @param property property to retrieve
Expand Down
2 changes: 1 addition & 1 deletion packages/webpack.web.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module.exports = (env, argv) => {
* environments. The following allows compilation to pass in Web mode by never bundling the module in the final output for web mode.
*/
new webpack.IgnorePlugin({
resourceRegExp: /httpResourceFetcher/, // matches the path in the require() statement
resourceRegExp: /node\/httpResourceFetcher/, // matches the path in the require() statement
}),
/**
* HACK: the ps-list module breaks Web mode if imported, BUT we still dynamically import this module for non web mode
Expand Down
Loading