luxon Convert codebase to TypeScript

This is a Work In Progress.

Trying to move the code base to TypeScript.

Before I go any further, I would like your thoughts on how the code looks, and if you would be willing to merge such a PR.

There are a couple of TypeScript specific Partial,Record constructions, but the rest of the code should be fairly easy to read. I personally appreciate the safety it provides. It already helped to find a few minor issues (that I should translate to tests anyway).

Jul 29 '19 11:07 GillesDebunne

This does look much better than I expected. I still don't like the little intermediate objects, but I think I can live with that. Let's do it

BTW, you've been so helpful. Would you like member access?

Aug 02 '19 19:08 icambron

This does look much better than I expected. I still don't like the little intermediate objects, but I think I can live with that. Let's do it

What do you mean by intermediate objects, the type definitions? Happy that you like it, I'll keep on working on it.

BTW, you've been so helpful. Would you like member access?

I'd be honored, thanks for the offer. I'm starting a new gig next month, and I don't know if I'll have a lot of spare time, but I'll be happy to help.

Gilles

Aug 03 '19 19:08 GillesDebunne

I mean the type definitions for things we pass around inside Luxon, like GregorianDateTime. Seems like an unfortunate amount of ceremony and it's what made me give up last time on Flow. But I think I can live with it.

Aug 04 '19 18:08 icambron

This latest commit converts the whole code base to TS, and the ~700 initial error messages are gone.

It mainly consists in adding types to all function parameters, and defining a few custom types along the way. I started from the work done by @mastermatt, @milesj and others on DefinitelyTyped.

In some cases, I had to refactor some code to help TS understand it, other times I carefully added some as keywords to force some types that could not be inferred. This process also helped me find a few minor bugs / typos (some of which deserve a dedicated test). For all (I hope) these non-trivial changes, I added a temporary comment with GILLES that you can look for in the review process.

Next steps:

make the rollup build process understand and manage TS
convert the tests to TS. Some tests with invalid input will be rejected by TS, figure how to handle these
make sure the index.d.ts is created as part of the release

Side note, with the new nullOnInvalid policy, all constructors now return a SomeObj | null type, even if nullOnInvalid is set to false (default). This can become annoying as one for instance has to write DateTime.local() as DateTime to enforce the type. I could not find a way to use overloads to change the return type based on the value of this parameter...

Aug 25 '19 21:08 GillesDebunne

👏

Aug 25 '19 22:08 milesj

Gilles, you can merge this whenever you're happy with it

Aug 26 '19 22:08 icambron

What's the release plan for this?

Aug 27 '19 17:08 mastermatt

This PR still needs some work. Making rollup understand TS was a breeze, but I now have 1000+ errors in the tests, which challenge my choices in types...

The major feature left in the 2.0 roadmap is to extract the parser/formatter to reduce the library's footprint.

Aug 27 '19 19:08 GillesDebunne

@GillesDebunne I was thinking about that. I don't currently have the bandwidth to take on the formatter separation; things are very busy at work. So unless you want to take that on too, I'm starting to think we should just release 2.0 without it and then do it for 3.0, with a timeline of "whenever we make the change". Otherwise, my worry is that all these awesome changes already in 2.0 will lie around when they could be valuable now (or, whenever the typescript rework is done).

Aug 28 '19 03:08 icambron

This is a new iteration of this PR. This time, all the tests were migrated to TS. This resulted in some changes in the API types, and revealed a bug introduced while refactoring to TS.

There are two breaking changes that need to be validated:

Settings.defaultZone setter was replaced by a Settings.setDefaultZone() method. This was done because of a TS limitation, where setters and getters have to use the same type. However in that case, the setter is much more permissive (number for UTC offset, string for IANA, as well as null and undefined for default) than the getter (always returns a Zone). At first I tried to fight and bypass this, but a comment in a thread made me realize that:

Setting.defaultZone = value;
expect(Setting.defaultZone).toBe(value); // fails, unless value was a Zone object

might indeed be surprising and confusing. This is not the behavior you get with the other fields (Settings.defaultLocale, Settings.defaultNumberingSystem, or Settings.defaultOutputCalendar), and a dedicated setter and its documentation will make it clearer I think.

Two tests around prototypes were disabled. They stated that DateTime/Duration prototype properties should not throw when accessed, and required adding optional chaining in getters, since the private data fields are not initialized in the prototype. They are easy to restore if needed, replacing return this.c.year by return this.c ? this.c.year : undefined (optional chaining is not yet available in TS). My question is, must we really enforce this property on the prototypes, what's the use case?

Next is a cleanup of the build process, so that it generates the index.d.ts declaration, and adding tests around code I modified. I have a list of other changes but they can go in a different PR.

Sep 06 '19 09:09 GillesDebunne

The asymmetry between the getter and setter makes me want the explicit method more, not less. But why don't we just have both? Settings.setDefaultLocal(anyOfSeveralDifferentTypes) and Settings.defaultLocale = hasToBeAZone. Then we get the flexibility and the symmetry.

For your second question, see #182

Sep 06 '19 13:09 icambron

New version of this PR. This time the goal was to get a green CI build.

Fixed eslint config and errors, and built the docs from a transpiled version of the sources to ES6.

Also (finally) created a new luxon.d.ts type definition file, which sadly required a lot of manual work (@mastermatt is that expected, or am I missing a tool here?). The definitions are close to what is on definitivelyTyped, with some subtle changes here and there.

This is getting close. I need to remove all the // GILLES comments, create new tests where the behavior changed, and list the API breaking changes I introduced.

Sep 08 '19 23:09 GillesDebunne

I'm a little confused by the need for a types file. Seems like that would just come for free?

Sep 08 '19 23:09 icambron

It mostly does, but the output is made of several files that need to be merged. Some 'public' methods such as friendlyX must be removed. It could probably be automated...

Sep 09 '19 00:09 GillesDebunne

@GillesDebunne I haven't been rebasing 2.0 to prevent this change from getting more complicated than it has to be, so I think the plan will be to merge this, then retrofit in the relevant changes from 1.x, and then release.

Sep 21 '19 20:09 icambron

This last commit removes the comments I had left around my changes. I double checked and reverted one.

I'll take a look at the rebase from master.

Sep 21 '19 21:09 GillesDebunne

What’s the status on this?

Jan 25 '20 19:01 kbradl16

The branch needs a massive rebase on the current master. I hope I will find time to do it, or someone can help.

On Sat, Jan 25, 2020 at 8:42 PM kbradl16 [email protected] wrote:

What’s the status on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/moment/luxon/pull/548?email_source=notifications&email_token=AAOPFGTYBC76NNBEQSV67H3Q7SI3DA5CNFSM4IHRODMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ5D5ZA#issuecomment-578436836, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOPFGXONWMUQVATUO7UQNLQ7SI3DANCNFSM4IHRODMA .

Jan 25 '20 20:01 GillesDebunne

I needed a break and decided to work on this PR over the weekend. It is now rebased on the latest 2.0 branch, which is close to master.

Some tests fail with a message like:

    Expected: "in 2 years"
    Received: "in 2 yr."

but it may be related to my node version (v12.16.1) [EDIT same errors with Travis]

The next step could be to give tsdx a try, since it handles TS libraries compilation using rollup.

May 04 '20 23:05 gdebunne

@GillesDebunne Yeah, I merged master in over the weekend. 2.0-take-2 should be all caught up. I am running Node v13 and the 2.0 branch tests pass for me, so I suspect you're right about the Node version.

May 05 '20 02:05 icambron

Fixed the relativeTime tests

May 05 '20 21:05 gdebunne

Thanks for all your effort ( +1 for TS :-D )... Even if I'm not sure if this kind of BigBang migration is the best way to do it... TypeScript can be used to compile/check mixed JS/TS code bases (using allowJs and checkJs - for additional JS checks). That way you could add the TypeScript compiler and than migrate the source code to TS incrementally.

https://www.typescriptlang.org/docs/handbook/migrating-from-javascript.html https://medium.com/@clayallsopp/incrementally-migrating-javascript-to-typescript-565020e49c88

May 06 '20 08:05 drothmaler

Even if I'm not sure if this kind of BigBang migration is the best way to do it...

Indeed, but now that it's done, we have a global picture of the changes. It revealed a couple of possible bugs on edge cases that I will investigate.

May 07 '20 08:05 gdebunne

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
10 out of 11 committers have signed the CLA.

:white_check_mark: icambron
:white_check_mark: downace
:white_check_mark: saltire
:white_check_mark: visualjerk
:white_check_mark: ryota-ka
:white_check_mark: jonknowles
:white_check_mark: GillesDebunne
:white_check_mark: gdebunne
:white_check_mark: anthonkendel
:white_check_mark: benkelaar
:x: dependabot[bot]