-
Notifications
You must be signed in to change notification settings - Fork 71
feat!: replace make-fetch-happen with ky and refresh tests/deps #784
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
base: main
Are you sure you want to change the base?
Changes from 20 commits
0048dc6
7e5149b
09c2cf5
4b527b2
dd74dcb
15b82cd
cb6875f
6ffc01c
b5882b0
58eea3c
7b59ffe
3ff32ad
f2864c6
f102c22
4a71bc6
b21b89a
cd8d6e3
c7371ae
ddb41ac
2c2f517
6ce22fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { readFileSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import semver from 'semver'; | ||
|
|
||
| const packageJsonPath = join(__dirname, '..', 'package.json'); | ||
|
|
||
| const resolveSpecVersion = (): string | undefined => { | ||
| try { | ||
| const raw = readFileSync(packageJsonPath, 'utf8'); | ||
| const packageJson = JSON.parse(raw) as { | ||
| dependencies?: Record<string, string>; | ||
| devDependencies?: Record<string, string>; | ||
| }; | ||
|
|
||
| const specDependencyVersion = | ||
| packageJson.dependencies?.['@unleash/client-specification'] ?? | ||
| packageJson.devDependencies?.['@unleash/client-specification']; | ||
|
|
||
| if (!specDependencyVersion) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (semver.valid(specDependencyVersion)) { | ||
| return specDependencyVersion; | ||
| } | ||
|
|
||
| if (semver.validRange(specDependencyVersion)) { | ||
| return semver.minVersion(specDependencyVersion)?.version; | ||
| } | ||
|
|
||
| return semver.coerce(specDependencyVersion)?.version; | ||
| } catch (_err: unknown) { | ||
| // Ignore filesystem/parse errors and fall back to undefined. | ||
| return undefined; | ||
| } | ||
| }; | ||
|
|
||
| export const supportedClientSpecVersion = resolveSpecVersion(); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import type { Agent } from 'node:http'; | ||
| import type { URL } from 'node:url'; | ||
| import type { Dispatcher } from 'undici'; | ||
|
|
||
| export interface HttpOptions { | ||
| agent?: (url: URL) => Agent; | ||
| dispatcher?: (url: URL) => Dispatcher; // this is a breaking change from 'agent'. Ref: https://github.com/Unleash/unleash-node-sdk/pull/332 | ||
| rejectUnauthorized?: boolean; | ||
| maxRetries?: number; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ import { getAppliedJitter } from './helpers'; | |
| import type { HttpOptions } from './http-options'; | ||
| import type { CollectedMetric, ImpactMetricsDataSource } from './impact-metrics/metric-types'; | ||
| import { SUPPORTED_SPEC_VERSION } from './repository'; | ||
| import { post } from './request'; | ||
| import { createHttpClient, type HttpClient } from './request'; | ||
| import { resolveUrl, suffixSlash } from './url-utils'; | ||
|
|
||
| export interface MetricsOptions { | ||
|
|
@@ -109,14 +109,12 @@ export default class Metrics extends EventEmitter { | |
|
|
||
| private customHeadersFunction?: CustomHeadersFunction; | ||
|
|
||
| private timeout?: number; | ||
|
|
||
| private httpOptions?: HttpOptions; | ||
|
|
||
| private platformData: PlatformData; | ||
|
|
||
| private metricRegistry?: ImpactMetricsDataSource; | ||
|
|
||
| private httpClientPromise: Promise<HttpClient>; | ||
|
|
||
| constructor({ | ||
| appName, | ||
| instanceId, | ||
|
|
@@ -145,11 +143,16 @@ export default class Metrics extends EventEmitter { | |
| this.headers = headers; | ||
| this.customHeadersFunction = customHeadersFunction; | ||
| this.started = new Date(); | ||
| this.timeout = timeout; | ||
| this.bucket = this.createBucket(); | ||
| this.httpOptions = httpOptions; | ||
| this.platformData = this.getPlatformData(); | ||
| this.metricRegistry = metricRegistry; | ||
| this.httpClientPromise = createHttpClient({ | ||
| appName, | ||
| instanceId, | ||
| connectionId, | ||
| timeout, | ||
| httpOptions, | ||
|
Comment on lines
+150
to
+154
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turned this into an immutable configuration per client. Maybe we should still enable different timeouts per request, and perhaps some httpOptions, but I also don't think we need them |
||
| }); | ||
| } | ||
|
|
||
| private getAppliedJitter(): number { | ||
|
|
@@ -206,15 +209,11 @@ export default class Metrics extends EventEmitter { | |
| const headers = this.customHeadersFunction ? await this.customHeadersFunction() : this.headers; | ||
|
|
||
| try { | ||
| const res = await post({ | ||
| const httpClient = await this.httpClientPromise; | ||
| const res = await httpClient.post({ | ||
| url, | ||
| json: payload as unknown as Record<string, unknown>, | ||
| appName: this.appName, | ||
| instanceId: this.instanceId, | ||
| connectionId: this.connectionId, | ||
| headers, | ||
| timeout: this.timeout, | ||
| httpOptions: this.httpOptions, | ||
| }); | ||
| if (!res.ok) { | ||
| // status code outside 200 range | ||
|
|
@@ -261,16 +260,12 @@ export default class Metrics extends EventEmitter { | |
| const headers = this.customHeadersFunction ? await this.customHeadersFunction() : this.headers; | ||
|
|
||
| try { | ||
| const res = await post({ | ||
| const httpClient = await this.httpClientPromise; | ||
| const res = await httpClient.post({ | ||
| url, | ||
| json: payload as unknown as Record<string, unknown>, | ||
| appName: this.appName, | ||
| instanceId: this.instanceId, | ||
| connectionId: this.connectionId, | ||
| interval: this.metricsInterval, | ||
| headers, | ||
| timeout: this.timeout, | ||
| httpOptions: this.httpOptions, | ||
| }); | ||
| if (!res.ok) { | ||
| if (res.status === 403 || res.status === 401) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,7 @@ | ||
| import { promises } from 'node:fs'; | ||
| import fetch from 'make-fetch-happen'; | ||
| import type { ClientFeaturesResponse, FeatureInterface } from '../feature'; | ||
| import type { CustomHeaders } from '../headers'; | ||
| import { buildHeaders } from '../request'; | ||
| import { createHttpClient } from '../request'; | ||
| import type { Segment } from '../strategy/strategy'; | ||
|
|
||
| export interface BootstrapProvider { | ||
|
|
@@ -29,11 +28,12 @@ export class DefaultBootstrapProvider implements BootstrapProvider { | |
|
|
||
| private segments?: Segment[]; | ||
|
|
||
| private appName: string; | ||
|
|
||
| private instanceId: string; | ||
|
|
||
| constructor(options: BootstrapOptions, appName: string, instanceId: string) { | ||
| constructor( | ||
| options: BootstrapOptions, | ||
| readonly appName: string, | ||
| readonly instanceId: string, | ||
| readonly connectionId: string, | ||
| ) { | ||
| this.url = options.url; | ||
| this.urlHeaders = options.urlHeaders; | ||
| this.filePath = options.filePath; | ||
|
|
@@ -45,21 +45,13 @@ export class DefaultBootstrapProvider implements BootstrapProvider { | |
| } | ||
|
|
||
| private async loadFromUrl(bootstrapUrl: string): Promise<ClientFeaturesResponse | undefined> { | ||
| const response = await fetch(bootstrapUrl, { | ||
| method: 'GET', | ||
| const httpClient = await createHttpClient({ | ||
| appName: this.appName, | ||
| instanceId: this.instanceId, | ||
| connectionId: this.connectionId, | ||
| timeout: 10_000, | ||
| headers: buildHeaders({ | ||
| appName: this.appName, | ||
| instanceId: this.instanceId, | ||
| etag: undefined, | ||
| contentType: undefined, | ||
| custom: this.urlHeaders, | ||
| }), | ||
| retry: { | ||
| retries: 2, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 is default retry |
||
| maxTimeout: 10_000, | ||
| }, | ||
| }); | ||
| const response = await httpClient.get({ url: bootstrapUrl, headers: this.urlHeaders }); | ||
| if (response.ok) { | ||
| return response.json(); | ||
| } | ||
|
|
@@ -90,6 +82,10 @@ export function resolveBootstrapProvider( | |
| options: BootstrapOptions, | ||
| appName: string, | ||
| instanceId: string, | ||
| connectionId: string, | ||
| ): BootstrapProvider { | ||
| return options.bootstrapProvider || new DefaultBootstrapProvider(options, appName, instanceId); | ||
| return ( | ||
| options.bootstrapProvider || | ||
| new DefaultBootstrapProvider(options, appName, instanceId, connectionId) | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ export class StreamingFetcher extends EventEmitter implements FetcherInterface { | |
|
|
||
| private readonly headers?: Record<string, string>; | ||
|
|
||
| private readonly connectionId?: string; | ||
| private readonly connectionId: string; | ||
|
|
||
| private readonly onSave: StreamingFetchingOptions['onSave']; | ||
|
|
||
|
|
@@ -30,8 +30,8 @@ export class StreamingFetcher extends EventEmitter implements FetcherInterface { | |
| url, | ||
| appName, | ||
| instanceId, | ||
| headers, | ||
| connectionId, | ||
| headers, | ||
| eventSource, | ||
| maxFailuresUntilFailover = 5, | ||
| failureWindowMs = 60_000, | ||
|
|
@@ -178,8 +178,7 @@ export class StreamingFetcher extends EventEmitter implements FetcherInterface { | |
| instanceId: this.instanceId, | ||
| etag: undefined, | ||
| contentType: undefined, | ||
| custom: this.headers, | ||
| specVersionSupported: '5.2.0', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was always out of sync so I added client-spec-version.ts helper (please review it) |
||
| headers: this.headers, | ||
| connectionId: this.connectionId, | ||
| }), | ||
| readTimeoutMillis: 60000, | ||
|
|
||
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.
Breaking change documentation: While the inline comment mentions this is a breaking change and references PR #332, consider documenting this breaking change more prominently in the PR description or changelog. The change from
agent(which used Node.js http/https Agent) todispatcher(which uses undici Dispatcher) requires users to update their code.Additionally, consider adding a deprecation period or providing a migration guide for users who were using the
agentoption.