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

Convert codebase to TypeScript (continued) #1047

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugofpsilva
Copy link

@hugofpsilva hugofpsilva commented Oct 6, 2021

Hello everyone!

Since @GillesDebunne seems to be AWOL I decided to try and continue his work. I hope he doesn't mind.

Let me start by saying that if his work was a WIP (and it was fantastic), this is a HUGE WIP and will be a major headache to anyone reviewing. I'd review everything, because
I actually have no idea what I'm doing 😅 This edit works for me, but I'm not sure it would work for everyone else.
Sorry 😄

I would have picked up where Gilles left off, weren't it for the dozens of commits on master since he last worked on it.
So I actually restarted from master, and using most of his work and some tweaks of my own and from the folks over at DefinitelyTyped, I finally arrived at no errors with all tests passing.

This is still 99% @GillesDebunne's work and credit for the migration should still go to him.
I just "rebased" and most probably introduced some bugs 🙄 🆘

Notable changes from master:

  1. Codebase migrated to TS;
  2. Added eslint (can ofc remove if this is undesired);
  3. Switched to benny for benchmarking. The benchmark package hasn't been updated in 5 years and benny seems to work better with TS projects, being very similar.

Notable changes from Gilles's work:

  1. Dropped tsdx. I don't really feel comfortable with this library. It hasn't been released, hasn't been updated in a year and uses quite a few deprecated packages. We could still try to fit this in if really needed. Downside of not using: see below.

Still to do (guide me oh wise ones 🙏):

  1. Build process. I was really hoping we could avoid more build tools other than tsc, but if we want to keep broad coverage probably other tools will have to be introduced. Suggestions?
  2. Check, double check and triple check everything!;
  3. I didn't touch the scripts folder, husky or codecov. Do these need tweaking as well?

Thanks for your time! 🤗

Hugo

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 6, 2021

CLA Signed

The committers are authorized under a signed CLA.

@icambron
Copy link
Member

Looking good @hugofpsilva!

@hugofpsilva
Copy link
Author

Where do we go from here?

@icambron
Copy link
Member

@hugofpsilva Sorry for the very slow reply, haven't had much Luxon time.

I'd like the next major version of Luxon to be Typescript based. I'll certainly do a full review of this, but first, how close is it to being done?

Ideally, this is how we do it:

  1. create a 3.0 branch and make this its head
  2. forward port any recent changes
  3. get it out as quickly as possible to prevent further drift

@likern
Copy link

likern commented Jan 12, 2022

@icambron Hello 👋 Are you still interested in this rewrite?

I've forked luxon project to address this issue #1112. I need not only half-opened intervals, but closed intervals support.

Along the way I migrated to Typescript (with huge rewrite to statically verify types).

If with new major version we could expand luxon functionality too it would be cool.

Also I've found that idea of Duration.invalid() is not good. It easily swallowed errors (if they are not expected to be thrown) when apply changed function which return boolean, for example. It always returns default false and information that error happened is lost.

@hugofpsilva
Copy link
Author

@icambron I haven't had much time for this either unfortunately. From where I was before, I was only lacking a proper build tool unless you want to ship only TS with no pure JS support. Now I believe this code has also fallen behind relating to master. I also had this question in particular I didn't touch the scripts folder, husky or codecov. Do these need tweaking as well?

@likern If your version is an improvement over this one, by all means, jump the gun 😉

@likern
Copy link

likern commented Jan 12, 2022

Not an improvement of this PR, it's a complete rewrite from scratch.

@hugofpsilva
Copy link
Author

I understand, I meant an improvement to luxon.

@likern
Copy link

likern commented Jan 22, 2022

@icambron This is my ongoing effort likern/timely#4

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.

3 participants