refactor(utils): migrate fields and timestamp tests from Flow to Type… - #4848
bonchevskyi wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe change adds exported field-selection collections and property-normalization helpers in ChangesField utilities
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The TypeScript migration preserves the utility and test contracts with no actionable regression identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. I nudge each field into its place Comment |
greg-in-a-box
left a comment
There was a problem hiding this comment.
Review
Straightforward Flow → TypeScript migration of fields, plus renaming the timestamp tests to .ts, consistent with the other utils migration batches.
What looks good
- Export surface matches master; field-fetch constant arrays and helper logic are preserved (function →
constarrow, docs trimmed). .js.flowstub via rename matches the established migration pattern for remaining Flow importers.findMissingProperties/fillMissingPropertiesoptional-param tidy (properties?:→propertieswith default;obj?:→objwith default) is type-only; runtime defaults behave the same.timestamp.test.tscasts (as unknown as number) correctly exercise the non-number path ofconvertTimestampToSecondsunder TypeScript.
Nits (non-blocking)
- CI (
lint_test_build) was still in progress at review time — worth confirming green before merge.
Verdict: Approve.
greg-in-a-box
left a comment
There was a problem hiding this comment.
Review
Mechanical Flow → TypeScript migration of fields, plus renaming the timestamp tests. Looks good to merge from a contract/behavior standpoint.
Checked
- Field-list constants (including archive variants) match
masterfields.jsone-for-one. findMissingProperties/fillMissingProperties/fillUserPlaceholderlogic and exports are unchanged.fields.js→fields.js.flowkeeps the full Flow copy for remaining Flow importers, consistent with the other in-flight utils migrations.fields.test.js→.tsis a rename-only;timestamp.test.tsonly adds the casts needed forconvertTimestampToSeconds(timestamp: number).
Nits (non-blocking)
- Helpers still use Flow-era
Object/Array<string>; fine for this pass, but a later cleanup could tighten toRecord<string, unknown>(or a shared shape) if you want real TS value from the types. as unknown as numberin the timestamp invalid-input cases is clear enough;@ts-expect-errorwould also document intent if you prefer that style.
Approving.
Summary
This PR finishes the remaining utils migration tail on
master:fieldsfrom Flow (.js) to TypeScript (.ts), plus renamingtimestamptests to TypeScript. Disjoint from other in-flight utils PRs.Migrated utilities
fieldsTests migrated (impl already TypeScript)
timestamp—timestamp.test.js→timestamp.test.ts(no change totimestamp.ts)Components / API modules touched (consumers)
No import path changes expected.
fieldsexports field-fetch constants and helpers (fillMissingProperties,fillUserPlaceholder,findMissingProperties, etc.) used across API/Elements code paths.Changes
fields.jstofields.tsand addedfields.js.flowfor remaining Flow importersfields.test.js→fields.test.tstimestamp.test.js→timestamp.test.tsContract
fields(exports and helper logic unchanged relative to the previous.jssource)Related
Browser/Cache/… throughXhr,uploads, etc.).js.flowstubs (and optional__mocks__/performance.js) remain undersrc/utils/for Flow compatibilitySummary by CodeRabbit
New Features
Tests