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

fix: Significantly improve Duration types #1338

Merged
merged 5 commits into from
Jan 17, 2021
Merged

fix: Significantly improve Duration types #1338

merged 5 commits into from
Jan 17, 2021

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jan 15, 2021

This PR improves Duration plugin types.

Why?

  • Autocomplete:

autocomplete

  • And for more strict checks (just compare):

pr-1

pr-2

New types

// That's how computed type looks like
type plugin.DurationUnitsObjectType = {
    milliseconds?: number;
    seconds?: number;
    minutes?: number;
    hours?: number;
    days?: number;
    months?: number;
    years?: number;
    weeks?: number;
}

I also added CreateDurationType to reduce code duplication, so duration(), duration.add() and duration.substract() using the same function signature.
Other changes you can view in diff. Please, let me know if don't understand something. I can explain every line of code.

UPD: With this PR TypeScript users can't use functions duration(), duration.add() and duration.substract() without args any more. But I don't think it's a breaking change, because I can't find any example usage in docs for this. In this way we just get empty duration object.

P.S. It also would be good to update TypeScript in devDependencies to latest version.

@zardoy zardoy marked this pull request as ready for review January 15, 2021 13:35
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #1338 (246705e) into dev (02d96ec) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #1338   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          176       176           
  Lines         1725      1725           
  Branches       394       394           
=========================================
  Hits          1725      1725           
Impacted Files Coverage Δ
src/locale/sl.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02d96ec...246705e. Read the comment docs.

@iamkun
Copy link
Owner

iamkun commented Jan 17, 2021

Thanks. Will the syntax require the latest Typescript run time? Cause I can see some of our users still using very old typescript like 1.x

@zardoy
Copy link
Contributor Author

zardoy commented Jan 17, 2021

Thank you for noticing that, but Day.js already uses Partial, that was introduced in TypeScript 2.1

export function locale(preset?: string | ILocale, object?: Partial<ILocale>, isLocal?: boolean): string

And unknown type, that was introduced in TypeScript 3.0
export function extend<T = unknown>(plugin: PluginFunc<T>, option?: T): Dayjs

But it is not a big problem because if TypeScript does not understand something it will fallback to any type.
Just compare computed variant of extend function:

// TypeScript 1.8.10 (TypeScript doesn't understand this Generic so it fallbacks to `any` type)
function extend<T, unknown>(plugin: any, option?: T): Dayjs
// TypeScript 2.9.2 (TypeScript still doesn't understand `unknown` type)
function extend<T = any>(plugin: PluginFunc<T>, option?: T): Dayjs
// TypeScript 3.0.0 and higher
function dayjs.extend<T = unknown>(plugin: PluginFunc<T>, option?: T): Dayjs

So the answer is: It will work, but with fallbacks to any type:

type plugin.CreateDurationType = ((units: any) => Duration) & ((time: number, unit?: any) => Duration) & ((ISO_8601: string) => Duration)

And only if the user uses TypeScript 2.x all the types will work correctly.
I think it would be good to encourage users to upgrade TypeScript version. Breaking changes for all TypeScript versions could be found here: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#typescript-20 .

@zardoy
Copy link
Contributor Author

zardoy commented Jan 17, 2021

Don't worry intellisense will work correctly (even if the user still uses TypeScript 1.x):
pr

@iamkun
Copy link
Owner

iamkun commented Jan 17, 2021

LGTM

@iamkun iamkun merged commit 4aca4b1 into iamkun:dev Jan 17, 2021
@iamkun
Copy link
Owner

iamkun commented Jan 17, 2021

Thank you, TS expert.

Just wondering if you could help us with another TS issue if you got some time.

#1281

iamkun pushed a commit that referenced this pull request Jan 22, 2021
## [1.10.4](v1.10.3...v1.10.4) (2021-01-22)

### Bug Fixes

* Correct handling negative duration ([#1317](#1317)) ([3f5c085](3f5c085))
* Improve `Duration` types ([#1338](#1338)) ([4aca4b1](4aca4b1))
* parse a string for MMM month format with underscore delimiter ([#1349](#1349)) ([82ef9a3](82ef9a3))
* Update Bengali [bn] locale ([#1329](#1329)) ([02d96ec](02d96ec))
* update locale Portuguese [pt] yearStart ([#1345](#1345)) ([5c785d5](5c785d5))
* update Polish [pl] locale yearStart ([#1348](#1348)) ([e93e6b8](e93e6b8))
* Update Slovenian [sl] relativeTime locale ([#1333](#1333)) ([fe5f1d0](fe5f1d0))
@iamkun
Copy link
Owner

iamkun commented Jan 22, 2021

🎉 This PR is included in version 1.10.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.10.4](iamkun/dayjs@v1.10.3...v1.10.4) (2021-01-22)

### Bug Fixes

* Correct handling negative duration ([#1317](iamkun/dayjs#1317)) ([3f5c085](iamkun/dayjs@3f5c085))
* Improve `Duration` types ([#1338](iamkun/dayjs#1338)) ([4aca4b1](iamkun/dayjs@4aca4b1))
* parse a string for MMM month format with underscore delimiter ([#1349](iamkun/dayjs#1349)) ([82ef9a3](iamkun/dayjs@82ef9a3))
* Update Bengali [bn] locale ([#1329](iamkun/dayjs#1329)) ([02d96ec](iamkun/dayjs@02d96ec))
* update locale Portuguese [pt] yearStart ([#1345](iamkun/dayjs#1345)) ([5c785d5](iamkun/dayjs@5c785d5))
* update Polish [pl] locale yearStart ([#1348](iamkun/dayjs#1348)) ([e93e6b8](iamkun/dayjs@e93e6b8))
* Update Slovenian [sl] relativeTime locale ([#1333](iamkun/dayjs#1333)) ([fe5f1d0](iamkun/dayjs@fe5f1d0))
andrewhood125ruhuc added a commit to andrewhood125ruhuc/SidRH2 that referenced this pull request May 10, 2022
## [1.10.4](iamkun/dayjs@v1.10.3...v1.10.4) (2021-01-22)

### Bug Fixes

* Correct handling negative duration ([#1317](iamkun/dayjs#1317)) ([3f5c085](iamkun/dayjs@3f5c085))
* Improve `Duration` types ([#1338](iamkun/dayjs#1338)) ([4aca4b1](iamkun/dayjs@4aca4b1))
* parse a string for MMM month format with underscore delimiter ([#1349](iamkun/dayjs#1349)) ([82ef9a3](iamkun/dayjs@82ef9a3))
* Update Bengali [bn] locale ([#1329](iamkun/dayjs#1329)) ([02d96ec](iamkun/dayjs@02d96ec))
* update locale Portuguese [pt] yearStart ([#1345](iamkun/dayjs#1345)) ([5c785d5](iamkun/dayjs@5c785d5))
* update Polish [pl] locale yearStart ([#1348](iamkun/dayjs#1348)) ([e93e6b8](iamkun/dayjs@e93e6b8))
* Update Slovenian [sl] relativeTime locale ([#1333](iamkun/dayjs#1333)) ([fe5f1d0](iamkun/dayjs@fe5f1d0))
splashwizard pushed a commit to splashwizard/tracking-time that referenced this pull request Oct 21, 2024
## [1.10.4](iamkun/dayjs@v1.10.3...v1.10.4) (2021-01-22)

### Bug Fixes

* Correct handling negative duration ([#1317](iamkun/dayjs#1317)) ([3f5c085](iamkun/dayjs@3f5c085))
* Improve `Duration` types ([#1338](iamkun/dayjs#1338)) ([4aca4b1](iamkun/dayjs@4aca4b1))
* parse a string for MMM month format with underscore delimiter ([#1349](iamkun/dayjs#1349)) ([82ef9a3](iamkun/dayjs@82ef9a3))
* Update Bengali [bn] locale ([#1329](iamkun/dayjs#1329)) ([02d96ec](iamkun/dayjs@02d96ec))
* update locale Portuguese [pt] yearStart ([#1345](iamkun/dayjs#1345)) ([5c785d5](iamkun/dayjs@5c785d5))
* update Polish [pl] locale yearStart ([#1348](iamkun/dayjs#1348)) ([e93e6b8](iamkun/dayjs@e93e6b8))
* Update Slovenian [sl] relativeTime locale ([#1333](iamkun/dayjs#1333)) ([fe5f1d0](iamkun/dayjs@fe5f1d0))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants