refactor: remove luxon package, replace with JS Intl library #12209
Conversation
wackerow
requested review from
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
github-actions
bot
added
dependencies 📦
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@wackerow Excellent! Seems we also need to remove |
|
Good catch; removed the types package... thanks @nhsz! |
Make lastUpdatedDate prop for Docs and Tutorials layouts required, remove force unwrap with !, use lastUpdatedDate as fallback for intlLastEdit
Temporary fix for `Export encountered errors on following paths: /_error: /500`
|
Getting a strange result where the lastDeployDate in the Footer is causing the build to fail with: Added a temporary debugging patch to check for paths starting with |
This reverts commit 57eb586.
getLocaleTimestamp requires a date string but was occasionally getting an empty string or undefined; this confirms a value exists for the argument before rendering the component that makes this function call
WalkthroughThe changes primarily focus on refining date handling across various components and layouts by eliminating the Changes
Assessment against linked issues
The changes effectively address the primary objective outlined in the linked issue by replacing Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
package.jsonis excluded by:!**/*.jsonyarn.lockis excluded by:!**/*.lock
Files selected for processing (8)
- src/components/CommunityEvents/index.tsx (8 hunks)
- src/components/Footer.tsx (1 hunks)
- src/layouts/Docs.tsx (3 hunks)
- src/layouts/Static.tsx (1 hunks)
- src/layouts/Tutorial.tsx (3 hunks)
- src/layouts/Upgrade.tsx (2 hunks)
- src/lib/utils/time.ts (1 hunks)
- src/pages/developers/tutorials.tsx (3 hunks)
Additional comments: 12
src/lib/utils/time.ts (1)
- 3-16: The refactoring of
getLocaleTimestampto useIntl.DateTimeFormatinstead ofluxonand the addition of anoptionsparameter for customization are good improvements. This change leverages native JavaScript capabilities, potentially simplifying the codebase and reducing dependency footprint. The implementation correctly handles the optionaloptionsparameter, providing a default format when not specified. This approach aligns well with best practices for flexibility and usability in utility functions.One minor suggestion is to ensure thorough testing of this function across different locales and options to verify the correctness of the date formatting in various use cases. Additionally, consider documenting the expected format of the
timestampstring and any limitations or considerations related to theIntl.DateTimeFormatusage, especially regarding browser compatibility.src/layouts/Static.tsx (1)
- 121-129: The conditional rendering of the last updated date in the
StaticLayoutcomponent based on the existence oflastUpdatedDateis a good practice. It ensures that the UI only displays relevant information when available, enhancing the user experience. The use ofgetLocaleTimestampfor formatting the date is consistent with the project's approach to date handling after the refactor.Ensure that the
lastUpdatedDateprop is properly passed and updated across all usages ofStaticLayoutto maintain the accuracy of the displayed date. Additionally, consider adding a brief comment explaining the conditional rendering logic for future maintainability.src/components/CommunityEvents/index.tsx (1)
- 31-45: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [34-52]
The refactoring in the
Eventcomponent to usegetLocaleTimestampfor date formatting and the removal of thelanguageprop in favor of using theuseRouterhook to get the locale are positive changes. These adjustments align with the overall goal of leveraging native JavaScript capabilities and simplifying the codebase. The use ofIntl.DateTimeFormatOptionsfor specifying the date format is appropriate and provides flexibility for different event date presentations.Ensure that the removal of the
languageprop and the reliance on theuseRouterhook do not introduce any regressions in how event dates are displayed across different locales. It might be beneficial to add tests or checks to verify the correct behavior in various locale settings.src/layouts/Upgrade.tsx (1)
- 197-202: The addition of conditional rendering for the last updated date in the
UpgradeLayoutcomponent based on the existence oflastUpdatedDateis a good practice, similar to the approach taken in other layouts. This ensures that the date is only displayed when available, maintaining a clean and relevant user interface.However, the
Skeletoncomponent import at the top of the file does not seem to be used within the provided code snippet. If theSkeletoncomponent is intended for use in parts of the layout not shown in the snippet, ensure its usage aligns with the loading state representation. If it's not being used, consider removing the import to keep the codebase clean and minimize bundle size.src/layouts/Tutorial.tsx (2)
- 168-176: The update to the
TutorialLayoutPropstype to include additional properties and the modification to the structure is a positive change, enhancing the type safety and clarity of the component's props. This change aligns with TypeScript best practices for defining component props more precisely.However, ensure that all usages of the
TutorialLayoutcomponent throughout the project have been updated to pass the required props according to the new type definition. This is crucial to avoid type errors and ensure the component functions as expected.
- 194-195: The update to the
intlLastEditassignment to uselastUpdatedDateifgitHubLastEdit.datais not available is a sensible fallback mechanism. This ensures that the component can still display a relevant last updated date even if the GitHub last edit information is not available, maintaining the accuracy and relevance of the information presented to the user.Consider adding a comment explaining the fallback logic for future maintainability, especially for developers who might not be familiar with the context behind the GitHub last edit feature.
src/layouts/Docs.tsx (2)
- 209-220: The change to make
lastUpdatedDatea required property in theDocsLayoutPropstype is a good practice, ensuring that all documents have a last updated date for transparency and user information. This aligns with the goal of maintaining accurate and up-to-date documentation.However, similar to the previous file, ensure that all usages of the
DocsLayoutcomponent throughout the project have been updated to pass thelastUpdatedDateprop. This is crucial for maintaining consistency and avoiding runtime errors due to missing required props.
- 236-237: The update to the
intlLastEditassignment to uselastUpdatedDateifgitHubLastEdit.datais not available is a sensible approach, ensuring that the component can display a relevant last updated date in all scenarios. This fallback mechanism enhances the robustness of the component by providing a default value when GitHub last edit information is not available.As mentioned previously, consider documenting this fallback logic for clarity and maintainability.
src/components/Footer.tsx (1)
- 301-306: The update to conditionally render the last deploy date in the
Footercomponent based on the existence oflastDeployDateis a good improvement. This change ensures that the deploy date is only displayed when available, which is a clean and user-friendly approach.Ensure that the
lastDeployDateprop is accurately passed to theFootercomponent in all relevant places within the project. Additionally, consider adding a brief comment explaining the conditional rendering logic for future maintainability.src/pages/developers/tutorials.tsx (3)
- 36-36: The import statement for
getLocaleTimestampcorrectly aligns with the PR's objective to replaceluxonwith native JavaScript solutions for date manipulation.- 129-132: The changes to the
publishedfunction, including the direct check for "Invalid Date", align well with the PR's objective and simplify the date handling logic by removing unnecessary constants.- 477-478: The integration of the
publishedfunction within theTutorialPagecomponent for conditional rendering of the published date is correctly implemented and aligns with the PR's objectives.
There was a problem hiding this comment.
Nice job! All looks fine @wackerow except this one in the tutorials page
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/pages/developers/tutorials.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pages/developers/tutorials.tsx
Description
getLocaleTimestamp.tsinsidesrc/lib/utils/time.tsto useIntl.DateTimeFormatinstead of luxon tooling.optionsprop here to override display options as-neededCommunityEvents/index.tsx, replacing with updated usage ofgetLocaleTimestampyarn remove luxon @types/luxonRelated Issue
https://www.notion.so/efdn/Review-if-we-can-replace-some-packages-with-native-implementation-1875f79b99f04555aa60bb5f7b14e8c0?pvs=4
Fixes #12019
Summary by CodeRabbit