luxon
luxon copied to clipboard
moment
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).
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?
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
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.
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.tsis 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...
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.
@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).
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.defaultZonesetter was replaced by aSettings.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 asnullandundefinedfor default) than the getter (always returns aZone). 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, replacingreturn this.c.yearbyreturn 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.
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
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.
I'm a little confused by the need for a types file. Seems like that would just come for free?
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...
@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.
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.
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 .
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.
@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.
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
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.
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]
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.
A careful rebase is required to make sure the new code from master is correctly typed
Only rebase? Because your wrote something has to be adapted? So you mean only regarding the rebase?
