Conversation
BREAKING CHANGE: Node.js >=16.0.0 and npm >=8.0.0 now required - Remove ^ from all dependency versions to prevent auto-updates - Upgrade TypeScript from 3.1.3 to 5.3.3 - Upgrade Jest from 23.6.0 to 29.7.0 with ts-jest integration - Upgrade axios from 0.19.2 to 1.7.0 - Upgrade all @types packages to latest versions - Remove deprecated preprocessor.js in favor of ts-jest - Update tsconfig.json for TS5 compatibility - Add logger module for improved debugging - Add IP normalization utilities - Enhance error handling with detailed error messages - Add new test files and remove deprecated ones
|
🚀 Beta версія опублікована: Встановити: npm install @evo/[email protected] |
src/client.ts
Outdated
| if (this.isDebug) { | ||
| log('INFO', `🚀 [FeatureFlags] Client starting...`); | ||
| log('INFO', `ℹ️ [FeatureFlags] URL: ${this.url}`); | ||
| log('INFO', `ℹ️ [FeatureFlags] Flags: ${Object.keys(this.defaultFlags).join(', ')}`); |
There was a problem hiding this comment.
not sure if this must be INFO log, maybe debug. it might be a lot of flags here
src/client.ts
Outdated
| public flag(flagName: string, ctx: any = {}): boolean { | ||
| const normalizedCtx = normalizeContext(ctx); | ||
|
|
||
| this.isDebug && log('INFO', `🎯 [FeatureFlags] Getting flag "${flagName}" with context:`, normalizedCtx); |
There was a problem hiding this comment.
its a bit strange that we log INFO if isDebug. maybe log('DEBUG') would be more appropriate
src/client.ts
Outdated
| const result = check ? check(normalizedCtx) : this.defaultFlags[flagName]; | ||
|
|
||
| if (this.isDebug) { | ||
| const emoji = result ? '✅' : '❌'; |
There was a problem hiding this comment.
I might be borring) but I see a little to no value in this emoji thing in log messages
src/client.ts
Outdated
| const response = await this.httpClient.callExchange(payload); | ||
| return response; | ||
| } catch (error) { | ||
| console.log('HTTP request failed or unexpected error occurred client'); |
There was a problem hiding this comment.
should use log() function here instead of console.log
src/example.ts
Outdated
| .catch((err) => console.log(`FF client start fail ${err}`)); | ||
| export type tFlagsService = Partial<typeof Flags>; | ||
|
|
||
| // Словник змінних для feature flags |
There was a problem hiding this comment.
Please, use English phrases in comments
src/http.ts
Outdated
| if (!baseURL) throw new Error('You should provide a valid base URL'); | ||
|
|
||
| this.httpClient = axios.create({ | ||
| baseURL: `${baseURL}/flags/load`, |
There was a problem hiding this comment.
/load must be called only once on client initialization (await client.start() is a good place)
/sync must be called in a loop
src/http.ts
Outdated
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| timeout: 3000, |
There was a problem hiding this comment.
timeout must be customizable with sane default (10 seconds is fine)
src/http.ts
Outdated
|
|
||
| public formatPayload(payload:any):any{ | ||
| return { | ||
| project: payload.project || "default_project", // Задайте значення за замовчуванням, якщо потрібно |
There was a problem hiding this comment.
project must be required. do not provide default value here
src/logger.ts
Outdated
| @@ -0,0 +1,5 @@ | |||
| export type LogType = 'INFO' | 'WARNING' | 'ERROR'; | |||
|
|
|||
| export function log(type: LogType, message: string, ...args: any[]): void { | |||
There was a problem hiding this comment.
If would be cool to provide Logger interface with default implementaion that uses console under the hood and so we can pass logevo in our internal services
src/utils.ts
Outdated
| const normalizedCtx: any = {}; | ||
|
|
||
| Object.keys(ctx).forEach((key) => { | ||
| if (key.endsWith('.ip') || key === 'ip') { |
There was a problem hiding this comment.
that is a bit optimistic way to assume that value must be ip if has those suffixes. I think the caller of this library must provide valid values themself and we as a library developers must not try to fix those values here
|
lgtm |
2 participants
No description provided.