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

front: add type-safe APIs for durations and distances #8816

Open
emersion opened this issue Sep 10, 2024 · 2 comments
Open

front: add type-safe APIs for durations and distances #8816

emersion opened this issue Sep 10, 2024 · 2 comments
Assignees
Labels
area:front Work on Standard OSRD Interface modules kind:enhancement Improvement of existing features

Comments

@emersion
Copy link
Member

Description and goal

We're planning to use Date instead of ad-hoc strings/numbers for timestamps for improved type safety. We don't have an equivalent dedicated type for durations and distances.

Upside of using a dedicated type is that mixups are prevented. Typically when doing math on duration/distance values, it can be easy to mix millimeters and kilometers, or millimeters and a number of elements. A dedicated type would make the TypeScript compiler complain about such mistakes.

Possible solution 1: TypeScript subtypes

See https://timjohns.ca/typescripts-hidden-feature-subtypes.html

type Distance = A & { readonly __type: unique symbol };

Downside: math on distances doesn't work, it returns a bare number.

const a = 42 as Distance;
const b = 43 as Distance;
const d: Distance = a + b; // error

To fix, we'd need to define helper functions such as:

const addDistances = (a: Distance, b: Distance) => a + b as Distance;

But it's quite tempting for users to just litter casts everywhere and loose the type safety benefits.

Possible solution 2: dedicated class

Introduce dedicated classes for durations and distances.

Similar to the upcoming temporal standard: https://tc39.es/proposal-temporal/docs/duration.html

class Duration {
  seconds: number; // XXX: should we use milliseconds instead?

  constructor(seconds: number) {
    this.seconds = seconds;
  }

  // Make comparison operators work on Duration objects (>, <, etc)
  valueOf() {
    return this.seconds;
  }

  toString() {
    return formatToISO(this.seconds);
  }

  add(other: Duration) {
    return new Duration(this.seconds + other.seconds);
  }
}

Downside: math operators don't work (+, -, etc), need to use the dedicated methods. But that's similar to Date here.

valueOf is possibly too tempting for users to perform math operations, so maybe should leave it out.

Acceptance criteria

.

@emersion emersion added kind:enhancement Improvement of existing features area:front Work on Standard OSRD Interface modules labels Sep 10, 2024
@emersion emersion self-assigned this Sep 10, 2024
emersion added a commit that referenced this issue Dec 3, 2024
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Dec 13, 2024
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Dec 23, 2024
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Dec 23, 2024
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 3, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 3, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 3, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 3, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 10, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
emersion added a commit that referenced this issue Jan 10, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
github-merge-queue bot pushed a commit that referenced this issue Jan 13, 2025
Signed-off-by: Simon Ser <[email protected]>
References: #8816
@emersion
Copy link
Member Author

The Duration constructor ought to be improved to make it clearer in which unit the number passed in is, and maybe make it possible to pass in numbers in other units (much like the temporal proposal linked above).

Ref #10230 (comment)

@emersion
Copy link
Member Author

Still TODO in this issue: introduce a type for distances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules kind:enhancement Improvement of existing features
Projects
None yet
Development

No branches or pull requests

1 participant