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

Allowing optional duration values in toSeconds #18

Merged
merged 2 commits into from
Nov 29, 2020

Conversation

ZigaStrgar
Copy link
Contributor

Motivation

Typescript declaration mentions that each of Durations interface properties is optional but the case in the code it's not the same. Thus not allowing you to use toSeconds as a standalone function without all of the properties present in the object.

Before change

toSeconds({ minutes: 3 }); // NaN
toSeconds({ minutes: 3, years: 0, months: 0, days: 0, hours: 0, seconds: 0, weeks: 0 }); // 180

After change

toSeconds({ minutes: 3 }); // 180
toSeconds({ minutes: 3, years: 0, months: 0, days: 0, hours: 0, seconds: 0, weeks: 0 }); // 180

I know this library servers another purpose but as it already contains such functionality and it is exposed to the user, why not make it as simple as possible and require as small object as possible to calculate the value.

@ZigaStrgar
Copy link
Contributor Author

@tolu

@tolu
Copy link
Owner

tolu commented Nov 27, 2020

Hey, thanks for the contribution! 🙏

I absolutely agree that this should be rectified.

I could imagine something like this, where we cover our backs for both cases of using any of the exports directly:

const defaultDuration = Object.freeze({
  years: 0,
  months: 0,
  weeks: 0,
  days: 0,
  hours: 0,
  minutes: 0,
  seconds: 0
});

export const toSeconds = (duration, startDate) => {
   duration = Object.assign({}, defaultDuration, duration)
   // ...
}
export const end = (duration, startDate) => {
  duration = Object.assign({}, defaultDuration, duration)
  // ...
}

What do you think @ZigaStrgar?

@ZigaStrgar
Copy link
Contributor Author

@tolu You are welcome and I absolutely agree with your proposition and I've already made a commit to reflect that.

@tolu tolu merged commit e154411 into tolu:master Nov 29, 2020
@ZigaStrgar ZigaStrgar deleted the feature/optional-duration-parameters branch May 7, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants