Convert codebase to TypeScript

[GillesDebunne]

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).

[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

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

[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

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

…

[icambron]

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.

[GillesDebunne]

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...

[milesj]

👏

[mastermatt]

Instead of using as here you could just set the return type on the function signature.

[GillesDebunne]

That's what I did initially. For consistency with the other methods, I then removed all the return types in function signatures (the only ones left are type guards).

If that's more idiomatic, I'll switch back to an explicit return type.

[mastermatt]

consistency should rule all

[GillesDebunne]

FYI, I reverted these and they are not identical.

Using return res as X in a function will indeed define the return type of the function as X. But it is brute force, and this as is way too strong since one can write return 42 as string.

On the other hand, specifying a return type on a method (when it cannot be properly inferred automatically) will also define the method's return type. But this time the compiler will simply check that the value returned indeed matches that type, which is what we usually want.

[mastermatt]

Was there a reason to use the Readonly type over just making the attr readonly?
private readonly ts: number;

[GillesDebunne]

That was done for consistency.

I understand primitives could support a readonly while Objects need a Readonly<> to have all their properties set to readonly.

I preferred to use the same keyword everywhere. Again, if this is not idiomatic, I'll be happy to fix.

[drothmaler]

@GillesDebunne Sorry, but I think you misunderstood the meaning/usage of the Readonly<T> Utility Type vs. the readonly keyword...

The readonly keyword is used to make a property readonly (but not it's value/content).

interface Bar { bazz: string }
class Foo {
    readonly n: number = 21
    readonly bar: Bar = { bazz: "foo" }
}
let foo = new Foo()
foo.n = 42 // <-- Error
foo.bar = { bazz: "bazz" } // <-- Error
foo.bar.bazz = "test" // <-- Working

The Readonly utility type (Readonly<T>) on the other hand modifies all properties of the type T to become readonly - e.g. to represent what Object.freeze does.

function freeze<T>(obj: T): Readonly<T>;

All Primitive values (like number, string, boolean, ...) already are immutable - they can't be altered. This means applying Readonly<T> to them (like Readonly<number> or Readonly<string>) has no effect at all!

interface Bar { bazz: string }
class Foo {
    n: number = 21
    i: Readonly<number> = 21  // makes no difference
    s: string = "foo"

    bar: Readonly<Bar> = { bazz: "foo" }  // basically the same as:
    bar2: { readonly bazz: string } = { bazz: "foo" }
}
let foo = new Foo()
foo.n = 42 // <-- Working
foo.i = 42 // <-- Working
foo.s = "bar" // <-- Working
foo.s.length = 1 // <-- Error

foo.bar = { bazz: "bazz" } // <-- Working
foo.bar.bazz = "test" // <-- Error
foo.bar2 = { bazz: "bazz" } // <-- Working
foo.bar2.bazz = "test" // <-- Error

So if you really want the whole thing to be readonly you need:

interface Bar { bazz: string }
class Foo {
    readonly n: number = 21
    readonly bar: Readonly<Bar> = { bazz: "foo" }
}
let foo = new Foo()
foo.n = 42 // <-- Error
foo.i = 42 // <-- Error
foo.bar = { bazz: "bazz" } // <-- Error
foo.bar.bazz = "test" // <-- Error

[gdebunne]

Thanks a lot, I thought I understood the difference between the two, and I chose to use Readonly everywhere for consistency. I did not realize it did not work as expected on primitives. Will fix.

[mastermatt]

export function maybeArray<T>(thing: T | ReadonlyArray<T>): T[] {

So the returned value is auto-typed based on the in the input.

[GillesDebunne]

Well spotted.

[icambron]

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

[mastermatt]

What's the release plan for this?

[GillesDebunne]

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.

[icambron]

@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).

[GillesDebunne]

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.

[icambron]

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

[GillesDebunne]

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.

[icambron]

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

[GillesDebunne]

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...

[icambron]

@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.

[GillesDebunne]

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.

[kbradl16]

What’s the status on this?

[GillesDebunne]

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 ***@***.***> wrote: What’s the status on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#548?email_source=notifications&email_token=AAOPFGTYBC76NNBEQSV67H3Q7SI3DA5CNFSM4IHRODMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ5D5ZA#issuecomment-578436836>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOPFGXONWMUQVATUO7UQNLQ7SI3DANCNFSM4IHRODMA> .

[jsf-clabot]

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.

✅ icambron
✅ downace
✅ saltire
✅ visualjerk
✅ ryota-ka
✅ jonknowles
✅ GillesDebunne
✅ gdebunne
✅ anthonkendel
✅ benkelaar
❌ dependabot[bot]

[loxy]

What is the status on this PR?

[GillesDebunne]

What is the status on this PR?

I do not have much time to spend on this project. It would need to be rebased and adapted first.

[loxy]

Ok, maybe I can help. Can you outline what is to be done (adapted)?

[GillesDebunne]

A careful rebase is required to make sure the new code from master is correctly typed

[loxy]

Only rebase? Because your wrote something has to be adapted? So you mean only regarding the rebase?

[hugofpsilva]

@GillesDebunne can I pick this from you?