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 moment with luxon in core/frontend #13648

Closed
ErisDS opened this issue Oct 22, 2021 · 5 comments
Closed

Replace moment with luxon in core/frontend #13648

ErisDS opened this issue Oct 22, 2021 · 5 comments
Labels
help wanted [triage] Ideal issues for contributors to help with stale [triage] Issues that were closed to to lack of traction

Comments

@ErisDS
Copy link
Member

ErisDS commented Oct 22, 2021

Ghost makes heavy use of the moment and moment-timezone libraries all over our code bases. However, it's an extremely heavy dependency, we have difficulties getting the timezone to "stick" and the project itself essentially recommends not using it anymore: https://momentjs.com/docs/#/-project-status/

Instead of using Moment, we want to change to using Luxon. We're using Luxon because of its timezone capabilities - something the core of Ghost makes heavy use of. In smaller packages where we only need to manipulate dates, rather than creating, storing and retrieving them, we'll use date-fns.

For the main Ghost codebase, @daniellockyer has already laid some groundwork, added Luxon as a dependency and ensured the default timezone is set to UTC.

From here, we need to start replacing usages of moment with luxon.

This is a new thing for us, we haven't used the library in many places yet and so we don't know a lot about it. For that reason, we want to start on areas of the codebase where we can do the least harm - the frontend. If we end up displaying dates incorrectly for some reason on the frontend that's much easier to fix than if we store the wrong date!

How to work on this issue

The slow and steady approach to refactoring is the only thing that really works. Branches should be very short lived (<1 day if possible). Therefore, please do not attempt to do this entire refactor in a single PR. It is not possible for us to review!

Instead, please find a single file inside of core/frontend, make the necessary changes, test and lint your work, then submit a PR. If you like, repeat that with another file.

Luxon provides a useful guide to help translate between code in moment and luxon, along with important functional differences.

Please follow our [contribution guidelines] closely, particularly being sure to reference this issue in your commit. Refactor commits should not use emojis as they are not user-facing changes.

Thank you 🙏 ❤️ 🙏

@ErisDS ErisDS added tests / tools / standards Hacktoberfest Issues suitable for hacktoberfest participants labels Oct 22, 2021
PakkuDon added a commit to PakkuDon/Ghost that referenced this issue Oct 22, 2021
refs: TryGhost#13648
Moment is in maintenance mode and is no longer recommended for use
bntzio added a commit to bntzio/Ghost that referenced this issue Oct 23, 2021
refs TryGhost#13648

We are replacing moment for luxon as it is now recommended.
bntzio added a commit to bntzio/Ghost that referenced this issue Oct 23, 2021
refs TryGhost#13648

We are replacing moment for luxon as it is now recommended.
ErisDS pushed a commit that referenced this issue Oct 28, 2021
refs: #13648

- We are replacing moment with luxon as it is now recommended.
@matthanley matthanley added help wanted [triage] Ideal issues for contributors to help with and removed Hacktoberfest Issues suitable for hacktoberfest participants labels Dec 1, 2021
danimajo added a commit to danimajo/Ghost that referenced this issue Dec 2, 2021
danimajo added a commit to danimajo/Ghost that referenced this issue Dec 3, 2021
danimajo added a commit to danimajo/Ghost that referenced this issue Dec 4, 2021
danimajo added a commit to danimajo/Ghost that referenced this issue Dec 4, 2021
@danimajo
Copy link
Contributor

@ErisDS @matthanley any feedback on my PRs?

@github-actions
Copy link
Contributor

Our bot has automatically marked this issue as stale because there has not been any activity here in some time.

The issue will be closed soon if there are no further updates, however we ask that you do not post comments to keep the issue open if you are not actively working on a PR.

We keep the issue list minimal so we can keep focus on the most pressing issues. Closed issues can always be reopened if a new contributor is found. Thank you for understanding 🙂

@github-actions github-actions bot added the stale [triage] Issues that were closed to to lack of traction label Apr 17, 2022
@ErisDS
Copy link
Member Author

ErisDS commented Apr 20, 2022

Hey @danimajo I still really want to get this done, but it turned out to require a lot more knowledge & context than I hoped. I don't think our tests are going to do a good job of picking up if formats change slightly. I also think we need more context of exactly which luxon functions to use where and why.

I know a couple of your PRs have now been closed by the stale bot, that's my bad. I need to look at them and provide feedback. The issue is that I don't have enough experience with luxon myself yet to know if the changes are correct :(

If there's anyone out there with a bunch of experience with it who'd be willing to help making changes, triaging PRs or just setting up some good example usages, I'd love to hear from you :)

@github-actions github-actions bot removed the stale [triage] Issues that were closed to to lack of traction label Apr 20, 2022
@github-actions
Copy link
Contributor

Our bot has automatically marked this issue as stale because there has not been any activity here in some time.

The issue will be closed soon if there are no further updates, however we ask that you do not post comments to keep the issue open if you are not actively working on a PR.

We keep the issue list minimal so we can keep focus on the most pressing issues. Closed issues can always be reopened if a new contributor is found. Thank you for understanding 🙂

@github-actions github-actions bot added the stale [triage] Issues that were closed to to lack of traction label Aug 19, 2022
@ErisDS
Copy link
Member Author

ErisDS commented Aug 23, 2022

I'm going to close this until we have a bit more context about how to do it & can support the effort.

If there's anyone intimately familiar with moment -> luxon migrations that's interested to help, I'm all 👂

@ErisDS ErisDS closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted [triage] Ideal issues for contributors to help with stale [triage] Issues that were closed to to lack of traction
Projects
None yet
Development

No branches or pull requests

3 participants