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

Add types from index to quarterOfYear #2823

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

bensaufley
Copy link
Contributor

Resolves an issue where a variable being passed in as unit that may conform to either the base function's expected type (e.g., ManipulateType) or the augmented function's type (QUnitType) would be considered a type violation:

type MyUnit = 'days' | 'weeks' | 'months' | 'quarters' | 'years';

const doThing = (date: Dayjs, unit: MyUnit) => {
  if (date.isSame(dayjs(), unit)) {
    return date.add(7, unit);
  }
  return date.subtract(3, unit);
};

The types as they stand currently have two overloads: one that takes the base types and one that takes the extended types; neither take both, when really, the plugin makes it so that the function can take both kinds. This causes an error when I may pass in 'quarters' or 'weeks' because QUnitType adds 'quarters' to UnitType and OpUnitType/ManipulateType adds 'weeks' but there is no type that has both.

This PR could also add this as a separate overload, if that were important. E.g.:

add(value: number, unit: QUnitType): Dayjs
add(value: number, unit: QUnitType | ManipulateType): Dayjs

But I don't think it is – I don't see any value in an overload only accepting QUnitType.

Another open question, I suppose: here are the types today:

  export type OpUnitType = UnitType | "week" | "weeks" | 'w';
  export type QUnitType = UnitType | "quarter" | "quarters" | 'Q';
  export type ManipulateType = Exclude<OpUnitType, 'date' | 'dates'>;

Should the QUnitType being passed to add/subtract also be Manipulated, excluding 'date' | 'dates'? Are 'date' | 'dates' valid values for add/subtract only when the plugin is added?

Maybe:

  export type OpUnitType = UnitType | "week" | "weeks" | 'w';
  export type QUnitType = UnitType | "quarter" | "quarters" | 'Q';
  export type ManipulateType = Exclude<OpUnitType, 'date' | 'dates'>;
  export type ManipulateQType = Exclude<QUnitType, 'date' | 'dates'>;

And maybe QUnitType should extend OpUnitType rather than UnitType? Which would make most of the changes in this PR obsolete, except that we'd replace add#unit/subtract#unit with ManipulateQType?

Resolves an issue where a variable being passed in as unit that may conform to *either* the base function's expected type (e.g., ManipulateType) *or* the augmented function's type (QUnitType) would be considered a type violation
@bensaufley
Copy link
Contributor Author

OK I've pushed an update after kind of rubber-duckying myself in the PR notes. Take a look at either commit and lmk which solution you prefer. I think I like the latest one best.

@bensaufley
Copy link
Contributor Author

It looks like Codecov is failing, not the tests? I don't think I've done anything there.

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.

1 participant