[@types/luxon] update types #22463
[@types/luxon] update types #22463
Conversation
types/luxon/index.d.ts
Outdated
| static defaultLocale: string; | ||
| static defaultNumberingSystem: string; | ||
| static defaultOutputCalendar: string; | ||
| static defaultZone: Zone; |
There was a problem hiding this comment.
Hah, I just saw this PR after I submitted one fixing the Settings. See #22494.
I defined it as a namespace to avoid the linting error preventing classes having only static methods, but I'm not sure this rule is a good one, especially since it forced me to define defaultZone as a const, although it's actually a read-only property.
This should be fixed here, as there is no setter for defaultZone in the JS code: see https://github.com/moment/luxon/blob/master/src/settings.js#L52
There was a problem hiding this comment.
I am thinking on this problem. How do you think about this?
class Settings {
static readonly defaultZone: Zone;
}Yes, I'm not sure too. about disabling lint rules is not the good way or not.
There was a problem hiding this comment.
I think a class is closer to how the JS code is actually designed, and a readonly property is closer to the reality than a const variable. That's how I originally designed Settings in my PR, but the linter shouted at me, and I didn't want to take the responsibility of removing the linting rule, not being the original author of the typing. But I personally don't have a problem with removing it.
There was a problem hiding this comment.
I'll remove my PR, BTW, since this one solves the problem I wanted to solve.
types/luxon/index.d.ts
Outdated
| static defaultZone: Zone; | ||
| static defaultZoneName: string; | ||
| static throwOnInvalid: boolean; | ||
| static now(): number; |
There was a problem hiding this comment.
now is a property, with a getter and a setter. See https://github.com/moment/luxon/blob/master/src/settings.js#L20. It should be defined as
static now: number;
There was a problem hiding this comment.
now is a callback for returning the current timestamp, so it should be a function. on default, now has defined in https://github.com/moment/luxon/blob/master/src/settings.js#L5
so it should be like static now: () => number
and i have a question. is this different with static now(): number?
There was a problem hiding this comment.
You're absolutely right, I misread the code. I'm not a specialist, but as I see it designed on the JS code, I think using
static now: () => number
would be clearer. It show that now is just a readable/writable property of type function that is meant to be replaced by another one, and not a static method of the class.
Since they're all new, they have a bug, that I fixed by using our own typings, copied from a PR that fiexes them. See DefinitelyTyped/DefinitelyTyped#22463
make Settings.defaultZone as readonly, change expression of Settings.now
Advantages: - cleaner immutable API - shorter bundle (-50KB from the gzipped bundle) Drawbacks: - not stable yet - no built-in formatting of durations Since typings are all new, they have a bug, that I fixed by using our own typings, copied from a PR that fiexes them. See DefinitelyTyped/DefinitelyTyped#22463
|
🔔 @jnizet - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
|
We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped! |
types/luxon/index.d.ts
Outdated
| throwOnInvalid: boolean; | ||
| resetCache(): void; | ||
| }; | ||
| class Settings { |
There was a problem hiding this comment.
WHy did you make this a class, and add a rule in the tslint.config?
There was a problem hiding this comment.
Lint rules saids Every member of this class is static. Use namespaces or plain objects instead.
For test, it should be namespace, not the type or interface.
But I thought the luxon code is written in class, so class seems to be right. Even there is rare possibility for Settings or Info class will be changed for having instance methods.
How do you think? namespace is more clearer than class with static?
There was a problem hiding this comment.
yeah, namespace is probably better, if/when an instance method is added we can make additional changes then.
|
@FourwingsY One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
|
@paulvanbrenk yup i changed to namespaces, and removed lint rule. |
|
🔔 @paulvanbrenk - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
|
@FourwingsY thanks for this, DateObjectUnits.month didn't work and I found your fix :). Waiting for merge:) |
|
Just a heads up, there's a small typo here: Missing the letter 'T' for DATETIME_HUGE & DATETIME_HUGE_WITH_SECONDS |
Please fill in this template.
npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.