Skip to content
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

Replace momentjs with dayjs where possible #699

Merged
merged 5 commits into from
Mar 24, 2022
Merged

Replace momentjs with dayjs where possible #699

merged 5 commits into from
Mar 24, 2022

Conversation

majakomel
Copy link
Contributor

@majakomel majakomel commented Mar 12, 2022

Related issue: #510

Also fixes an issue with inconsistent time display - described in: #515

Initially, I intended to go with date-fns, but unfortunately it doesn't have a nice interface to work with timezones/utc and it required working a lot with the native Date object, which defeats the purpose of a library altogether. After a bit of exploration and testing, I decided to go with day.js, which is very lightweight, just as popular, and with a very similar interface as momentjs - which makes it also easy to replace.

Unfortunately, it's not possible to get rid of moment.js completely just yet since the Datetime component depends on it. I will follow up with this issue, to replace it with a date-library-agnostic component and remove the use of momentjs completely in another PR.

@vercel
Copy link

vercel bot commented Mar 12, 2022

@majakomel is attempting to deploy a commit to the OONI Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 14, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/ooni/explorer/C23oejQFtoyrtNzgAcNEcHhqcTH2
✅ Preview: https://explorer-git-fork-majakomel-replace-momentjs-ooni1.vercel.app

@hellais
Copy link
Member

hellais commented Mar 14, 2022

Thanks for putting this together!

Left a comment on something that should be addressed prior to merge.

Also, while you are touching this bit of the codebase, perhaps you can also integrate a fix for this bug: #515 (comment)?

@majakomel
Copy link
Contributor Author

Thanks for the review @hellais, I fixed the issue with the timezone and inconsistent time display, can you check again if things are as expected?

@majakomel majakomel requested a review from hellais March 16, 2022 07:33
@hellais
Copy link
Member

hellais commented Mar 16, 2022

I did some testing of the branch and it works great.

I left 2 very minor comments in the PR, which once addressed we can merge it.

Thanks for putting this together!

@hellais
Copy link
Member

hellais commented Mar 24, 2022

Yes this looks great. Thanks for putting it together!

I am going to proceed with merging it.

@hellais hellais merged commit 0dbf173 into ooni:master Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants