-
Notifications
You must be signed in to change notification settings - Fork 356
Fix string interpolation from converting objects to [object Object] #9114
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
Fix string interpolation from converting objects to [object Object] #9114
Conversation
📝 WalkthroughSummary by CodeRabbitBug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR fixes string interpolation of objects and arrays in the Changes
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/string-interpolation/tests/parse-interpolation-strings.test.ts (1)
23-33: Good coverage for embedded object stringification.These tests properly validate that objects and arrays are JSON-stringified when embedded in larger strings.
Consider adding tests for additional edge cases:
Suggested additional test cases
it('handles multiple object placeholders in one string', () => { const context = { user: { name: 'John' }, role: { type: 'admin' } }; const result = stringInterpolator.parse('{user} has role {role}', context); expect(result).toBe('{"name":"John"} has role {"type":"admin"}'); }); it('handles nested objects', () => { const context = { user: { name: 'John', address: { city: 'NYC' } } }; const result = stringInterpolator.parse('User: {user}', context); expect(result).toBe('User: {"name":"John","address":{"city":"NYC"}}'); }); it('handles Date objects', () => { const date = new Date('2024-01-01T00:00:00Z'); const context = { timestamp: date }; const result = stringInterpolator.parse('{timestamp}', context); expect(result).toEqual(date); // returned directly }); it('handles Date objects in strings', () => { const date = new Date('2024-01-01T00:00:00Z'); const context = { timestamp: date }; const result = stringInterpolator.parse('Time: {timestamp}', context); expect(result).toBe(`Time: ${JSON.stringify(date)}`); });packages/string-interpolation/src/interpolator.js (1)
129-139: Consider adding error handling for JSON.stringify as defensive programming.The JSON.stringify call on line 137 could theoretically throw errors on circular references or BigInt values. While the test suite and actual usage patterns (headers, URLs, query parameters) don't demonstrate this occurring in practice, wrapping it in a try-catch with a fallback to String() would be more robust:
// For objects embedded in a larger string, JSON stringify them if (typeof modifiedData === 'object' && modifiedData !== null) { - return str.replace(rule.replace, JSON.stringify(modifiedData)); + try { + return str.replace(rule.replace, JSON.stringify(modifiedData)); + } catch (e) { + return str.replace(rule.replace, String(modifiedData)); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/nine-plums-ask.mdpackages/string-interpolation/src/interpolator.jspackages/string-interpolation/tests/parse-interpolation-strings.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit / node 20
- GitHub Check: integration / node 18
- GitHub Check: integration / node 22
- GitHub Check: e2e / node v22
- GitHub Check: e2e / node v18
- GitHub Check: e2e / node v20
- GitHub Check: unit / node 22
- GitHub Check: apollo-federation-compatibility
- GitHub Check: check
- GitHub Check: unit / node 18
- GitHub Check: integration / node 20
🔇 Additional comments (2)
packages/string-interpolation/tests/parse-interpolation-strings.test.ts (1)
11-21: Excellent test coverage for the direct return behavior.These tests clearly validate that objects and arrays are returned directly when the entire template is a single placeholder, which is the core feature of this PR.
.changeset/nine-plums-ask.md (1)
1-5: LGTM! Changeset is properly formatted.The changeset correctly documents this as a patch-level fix for the
@graphql-mesh/string-interpolationpackage with a clear description.
🚨 IMPORTANT: Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding. Failure to do so may result in the rejection of
the pull request.
Description
When using
stringInterpolator.parse()with an object or array value, the interpolation was producing[object Object]instead of preserving the object or serializing it properly.This change modifies the
applyRule()method ininterpolator.jsto:{user})User: {user})Fixes #9113
Type of change
expected)
Screenshots/Sandbox (if appropriate/relevant):
How Has This Been Tested?
Added unit tests in
packages/string-interpolation/tests/parse-interpolation-strings.test.ts:Test Environment:
@graphql-mesh/string-interpolation: latestChecklist:
CONTRIBUTING doc and the
style guidelines of this project
changeset using
yarn changesetthat bumps the versionFurther comments
This fix is particularly useful when using interpolation in configurations where you want to pass through object values rather than having them converted to strings. The implementation checks if the modified data is an object and handles it appropriately based on whether it's a standalone placeholder or embedded in text.