[Merged by Bors] - feat: improve variables utils (COR-9708) #592
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the variable utilities to improve the API design by consolidating parameters into an options object and simplifying internal logic.
Changes:
- Consolidated
modifierparameter intooptionsobject forreplaceVariablesfunction - Added default value for
modifierparameter invariableReplacerto eliminate runtime type checks - Extended
deepVariableSubstitutionto accept anoptionsparameter for better configurability - Introduced
formattedPhrasevariable to improve code clarity inreplaceVariables - Updated test to use the new API signature with
{ modifier: JSON.stringify }
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/common/src/utils/variables.ts | Refactored function signatures to use options object pattern, simplified modifier handling with default parameters, and improved code readability with extracted variables |
| packages/common/src/utils/variables.test.ts | Updated test case to use new API signature with modifier inside options object |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (typeof subCollection === 'string') { | ||
| return replaceVariables(subCollection, variableMap, modifier); | ||
| return replaceVariables(subCollection, variableMap, options as any); |
There was a problem hiding this comment.
The use of as any bypasses TypeScript's type checking. This could hide type incompatibilities between the options passed to deepVariableSubstitution and those expected by replaceVariables. Consider using a more specific type assertion or refactoring the type definitions to ensure type safety. The main concern is that keepTypeIfOnlyVariable: true in options would change the return type of replaceVariables to unknown, but this function is being used in a context where a string is expected, which could cause runtime issues.
| export function replaceVariables( | ||
| phrase: string | undefined | null, | ||
| variables: Record<string, unknown>, | ||
| modifier?: (variable: unknown) => unknown, | ||
| options?: { trim?: boolean; keepTypeIfOnlyVariable?: false } | ||
| options?: { modifier?: (variable: unknown) => unknown; trim?: boolean; keepTypeIfOnlyVariable?: false } |
There was a problem hiding this comment.
This change modifies the public API signature by moving the modifier parameter from being a separate third parameter to being part of the options object. This is a breaking change that will affect any external code calling replaceVariables with the old signature (e.g., replaceVariables(phrase, vars, modifierFn)). Consider providing backward compatibility or ensuring all call sites across the codebase and dependent packages have been updated. A deprecation path or migration guide may be helpful.
|
bors r+ |
|
Pull request successfully merged into master. Build succeeded: |
bors-vf
bot
changed the title
No description provided.