Make moment an optional dependency #2220
Conversation
There was a problem hiding this comment.
Hi @frw,
Thanks a lot for the contribution!
I've added a small comment (please let me know if you do not have the capacity to fix that).
In any case we cannot merge this into master IMO since it's a breaking change, but we should definitely merge it for the next major release.
| return value.toLocaleDateString(); | ||
| } | ||
| if (!moment) { | ||
| console.error(`RNUILib DateTimePicker component with dateFormat requires installing "moment" dependency`); |
There was a problem hiding this comment.
I think adding this and the other error to a useEffect (either the existing one or a new one with [dateFormat, timeFormat] as a dependency array) will simplify the code and make it more readable.
There was a problem hiding this comment.
Hi @M-i-k-e-l,
Thanks for reviewing. Definitely makes sense to have it in the next major release.
I've made the changes to the code as you've suggested. Let me know if there's anything else you'd like me to change.
|
Internal note: look at unifying with #1805 |
|
Why not replace with a better option like https://day.js.org/ or even https://date-fns.org/ if dayjs are no longer enough. |
Description
This PR makes the moment package an optional dependency so as to decrease bundle size for those that do not use moment to format date/time.
Potentially breaking change: Projects that do not have moment as a direct dependency will find when upgrading that the supplied
dateFormat/timeFormatwould no longer have any effect on the displayed date/time, although there will be an error message logged to console to indicate that this is happening.Tested by running the example app and ensured the dates/times are still rendered properly
Changelog
Make moment an optional dependency