refactor(time): migrate ReadableTime from Flow to TypeScript - #4763
Conversation
WalkthroughAdded the internationalized ChangesReadableTime formatting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/components/time/index.js.flow (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: export the props type here for parity with
index.ts.
index.tsline 2 exportsReadableTimeProps. This Flow entry point exports only the component. Flow consumers cannot reach the props shape from the index.ReadableTime.js.flowalso keeps itsPropstype unexported, so exporting it there first is a prerequisite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/time/index.js.flow` around lines 1 - 3, Export the ReadableTime props type through the Flow entry point for parity with index.ts. First export the Props type from ReadableTime.js.flow, then re-export it alongside ReadableTime from the Flow index.src/components/time/ReadableTime.ts (2)
38-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
shouldShowYearafter the future-timestamp correction, and avoid reassigning the parameter.Line 39 computes
shouldShowYearfrom the uncorrectedtimestamp. Lines 41-44 then replacetimestamp. The derived value therefore describes a value that is no longer formatted. The corrected value is normally today, soisTodaywins andshouldShowYearis ignored in practice, but the ordering is fragile for later edits.Line 43 also reassigns a destructured parameter, which many ESLint configs reject under
no-param-reassign.♻️ Proposed refactor
const shouldUppercase = uppercase && !nonUppercaseLocales.includes(intl.locale); const relativeIfNewerThanTs = Date.now() - relativeThreshold; - const shouldShowYear = !isCurrentYear(timestamp); - - if (!allowFutureTimestamps && timestamp > Date.now()) { - // TODO: what is the reasoning behind this rule? - timestamp = relativeIfNewerThanTs; // Default to 'Today' for timestamps that would show a future date - } + + // Default to 'Today' for timestamps that would show a future date + // TODO: what is the reasoning behind this rule? + const effectiveTimestamp = + !allowFutureTimestamps && timestamp > Date.now() ? relativeIfNewerThanTs : timestamp; + + const shouldShowYear = !isCurrentYear(effectiveTimestamp);Then use
effectiveTimestampin the branches below and invalues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/time/ReadableTime.ts` around lines 38 - 44, In ReadableTime, stop mutating the destructured timestamp parameter by introducing an effectiveTimestamp value that applies the future-timestamp correction. Compute shouldShowYear with effectiveTimestamp after that correction, then replace subsequent timestamp usage in the formatting branches and values construction with effectiveTimestamp.
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
intlfrom the exportedReadableTimePropstype.
ReadableTimePropsis a consumer-facing public type, but the exported default isinjectIntl(ReadableTime), which suppliesintl. This makes TypeScript consumers who type props withReadableTimePropsadd an injected prop instead of describing the props the wrapped component accepts. Re-export own props plusWrappedComponentPropsfromreact-intlfor typed consumers while keeping the implementation prop shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/time/ReadableTime.ts` around lines 9 - 24, Remove intl from the exported ReadableTimeProps interface and preserve it only in the internal ReadableTime implementation props. Update the public export typing for the injectIntl-wrapped ReadableTime to combine the component’s own props with react-intl’s WrappedComponentProps, so consumers type only supplied props while the injected intl prop remains supported internally.src/components/time/ReadableTime.js.flow (1)
11-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider reducing this shim to declarations only.
This file repeats the whole component body from
ReadableTime.ts, including the branch chain and the threshold logic. Two copies of the same logic can drift as the TS file changes. A.js.flowsibling normally declares only the exported types and signatures, and the compiled TS output supplies the runtime behavior.If the repository convention for this migration is to keep the full body, ignore this note.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/time/ReadableTime.js.flow` around lines 11 - 38, The ReadableTime.js.flow shim duplicates the runtime implementation from ReadableTime.ts; reduce it to declarations only by retaining the Props type and exported component signature while removing the duplicated constants, destructuring, and function body. Follow the repository’s existing migration convention if sibling .js.flow files establish a different declaration pattern.src/components/time/ReadableTime.stories.tsx (1)
50-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the deprecated notes parameter with Docs metadata.
Storybook 10 moves story documentation into the docs parameters, while
@storybook/addon-notesdocumentation remains available separately. Type the default export asMetaif you keep Storybook CSF typing here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/time/ReadableTime.stories.tsx` around lines 50 - 59, Update the ReadableTime story’s default export to replace the deprecated parameters.notes metadata with the Storybook docs metadata structure, while preserving the existing notes content through the supported addon-notes configuration. Type the default export as Meta if CSF typing is used, and leave the existing chromatic setting unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/time/index.js.flow`:
- Around line 1-3: Export the ReadableTime props type through the Flow entry
point for parity with index.ts. First export the Props type from
ReadableTime.js.flow, then re-export it alongside ReadableTime from the Flow
index.
In `@src/components/time/ReadableTime.js.flow`:
- Around line 11-38: The ReadableTime.js.flow shim duplicates the runtime
implementation from ReadableTime.ts; reduce it to declarations only by retaining
the Props type and exported component signature while removing the duplicated
constants, destructuring, and function body. Follow the repository’s existing
migration convention if sibling .js.flow files establish a different declaration
pattern.
In `@src/components/time/ReadableTime.stories.tsx`:
- Around line 50-59: Update the ReadableTime story’s default export to replace
the deprecated parameters.notes metadata with the Storybook docs metadata
structure, while preserving the existing notes content through the supported
addon-notes configuration. Type the default export as Meta if CSF typing is
used, and leave the existing chromatic setting unchanged.
In `@src/components/time/ReadableTime.ts`:
- Around line 38-44: In ReadableTime, stop mutating the destructured timestamp
parameter by introducing an effectiveTimestamp value that applies the
future-timestamp correction. Compute shouldShowYear with effectiveTimestamp
after that correction, then replace subsequent timestamp usage in the formatting
branches and values construction with effectiveTimestamp.
- Around line 9-24: Remove intl from the exported ReadableTimeProps interface
and preserve it only in the internal ReadableTime implementation props. Update
the public export typing for the injectIntl-wrapped ReadableTime to combine the
component’s own props with react-intl’s WrappedComponentProps, so consumers type
only supplied props while the injected intl prop remains supported internally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c135d9e0-9db8-4599-9d0e-232dae622ef6
⛔ Files ignored due to path filters (1)
src/components/time/__tests__/__snapshots__/ReadableTime.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
src/components/time/ReadableTime.js.flowsrc/components/time/ReadableTime.stories.tsxsrc/components/time/ReadableTime.tssrc/components/time/__tests__/ReadableTime.test.tsxsrc/components/time/index.js.flowsrc/components/time/index.tssrc/components/time/messages.js.flowsrc/components/time/messages.ts
b3409ac to
18c00ad
Compare
Merge Queue Status
This pull request spent 24 minutes 9 seconds in the queue, including 11 minutes 47 seconds running CI. Required conditions to merge
|
3 participants
Convert ReadableTime component to TypeScript
This PR converts
src/components/timefrom JavaScript with Flow to TypeScript.Changes
ReadableTime.jstoReadableTime.tswith exportedReadableTimePropsinterfacemessages.jstomessages.tsindex.jstoindex.ts, re-exporting the component and its typesReadableTime.stories.jstoReadableTime.stories.tsx__tests__/ReadableTime.test.jstoReadableTime.test.tsx.js.flowfiles for backward compatibilityTesting
src/components/time; all 12 pass with snapshots matching previous outputyarn lint:tsandflow checkpassComponents/ReadableTime) that behavior is unchangedSummary by CodeRabbit
New Features
Documentation