Skip to content

Commit 41e35ff

Browse files
authored
refactor(core): Manifest/LSP fetching should use httpResourceFetcher (#6411)
## Problem Now that we've refactored the httpResourceFetcher a little bit we can use that for fetching both the manifest and the language server ## Solution --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 16d2bc2 commit 41e35ff

File tree

4 files changed

+9
-58
lines changed

4 files changed

+9
-58
lines changed

packages/core/src/shared/lsp/lspResolver.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import AdmZip from 'adm-zip'
1212
import { TargetContent, logger, LspResult, LspVersion, Manifest } from './types'
1313
import { getApplicationSupportFolder } from '../vscode/env'
1414
import { createHash } from '../crypto'
15-
import request from '../request'
15+
import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher'
1616

1717
export class LanguageServerResolver {
1818
constructor(
@@ -165,9 +165,8 @@ export class LanguageServerResolver {
165165
}
166166

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

packages/core/src/shared/lsp/manifestResolver.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55

66
import { getLogger } from '../logger/logger'
77
import { ToolkitError } from '../errors'
8-
import { RetryableResourceFetcher } from '../resourcefetcher/httpResourceFetcher'
98
import { Timeout } from '../utilities/timeoutUtils'
109
import globals from '../extensionGlobals'
1110
import { Manifest } from './types'
11+
import { HttpResourceFetcher } from '../resourcefetcher/httpResourceFetcher'
1212

1313
const logger = getLogger('lsp')
1414

@@ -40,14 +40,11 @@ export class ManifestResolver {
4040
}
4141

4242
private async fetchRemoteManifest(): Promise<Manifest> {
43-
const resourceFetcher = new RetryableResourceFetcher({
44-
resource: this.manifestURL,
45-
params: {
46-
timeout: new Timeout(manifestTimeoutMs),
47-
},
48-
})
43+
const resp = await new HttpResourceFetcher(this.manifestURL, {
44+
showUrl: true,
45+
timeout: new Timeout(manifestTimeoutMs),
46+
}).getNewETagContent(this.getEtag())
4947

50-
const resp = await resourceFetcher.getNewETagContent(this.getEtag())
5148
if (!resp.content) {
5249
throw new ToolkitError('New content was not downloaded; fallback to the locally stored manifest')
5350
}

packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts

Lines changed: 0 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -134,51 +134,6 @@ export class HttpResourceFetcher implements ResourceFetcher<Response> {
134134
}
135135
}
136136

137-
export class RetryableResourceFetcher extends HttpResourceFetcher {
138-
private readonly requestTimeoutMs: number
139-
private readonly retryIntervalMs: number
140-
private readonly resource: string
141-
142-
constructor({
143-
resource,
144-
params: { retryNumber = 5, retryIntervalMs = 3000, showUrl = true, timeout = new Timeout(5000) },
145-
}: {
146-
resource: string
147-
params: {
148-
retryNumber?: number
149-
retryIntervalMs?: number
150-
showUrl?: boolean
151-
timeout?: Timeout
152-
}
153-
}) {
154-
super(resource, {
155-
showUrl,
156-
timeout,
157-
})
158-
this.requestTimeoutMs = retryNumber * retryIntervalMs
159-
this.retryIntervalMs = retryIntervalMs
160-
this.resource = resource
161-
}
162-
163-
fetch(versionTag?: string) {
164-
return waitUntil(
165-
async () => {
166-
try {
167-
return await this.getNewETagContent(versionTag)
168-
} catch (err) {
169-
getLogger('lsp').error('Failed to fetch at endpoint: %s, err: %s', this.resource, err)
170-
throw err
171-
}
172-
},
173-
{
174-
timeout: this.requestTimeoutMs,
175-
interval: this.retryIntervalMs,
176-
retryOnFail: true,
177-
}
178-
)
179-
}
180-
}
181-
182137
/**
183138
* Retrieves JSON property value from a remote resource
184139
* @param property property to retrieve

packages/webpack.web.config.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ module.exports = (env, argv) => {
4141
* environments. The following allows compilation to pass in Web mode by never bundling the module in the final output for web mode.
4242
*/
4343
new webpack.IgnorePlugin({
44-
resourceRegExp: /httpResourceFetcher/, // matches the path in the require() statement
44+
resourceRegExp: /node\/httpResourceFetcher/, // matches the path in the require() statement
4545
}),
4646
/**
4747
* HACK: the ps-list module breaks Web mode if imported, BUT we still dynamically import this module for non web mode

0 commit comments

Comments
 (0)