adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.99k stars 1.13k forks source link

`parseDuration` prohibits carry over values #5416

Open mfwgenerics opened 12 months ago

mfwgenerics commented 12 months ago

Provide a general summary of the issue here

Valid ISO 8601 durations such as PT36H can not be parsed using parseDuration from @internationalized/date because 36 falls outside of the internal range constraint.

πŸ€” Expected Behavior?

parseDuration("PT36H")
{
   "years": 0,
   "months": 0,
   "weeks": 0,
   "days": 0,
   "hours": 36,
   "minutes": 0,
   "seconds": 0
}

😯 Current Behavior

parseDuration("PT36H")
Error: Invalid ISO 8601 Duration string: PT36H

πŸ’ Possible Solution

These limits could be removed:

https://github.com/adobe/react-spectrum/blob/f600fdfa2af9f3131b34b1536a790aa1bb348151/packages/%40internationalized/date/src/string.ts#L254

πŸ”¦ Context

This current behavior makes it difficult to use parseDuration when you are working with time units only.

I can't tell exactly how the standard treats the issue, but it does seem that there is no general conversion between units. This makes sense to me, because P1DT12H could represent a longer, shorter or equivalent length of time to PT36H depending on the start date time (due to daylight savings). I believe most other ISO 8601 duration implementations preserve the distinction between P1DT12H and PT36H.

πŸ–₯️ Steps to Reproduce

import React from "react";
import { Provider, defaultTheme, Button } from "@adobe/react-spectrum";
import "./styles.css";
import { parseDuration } from "@internationalized/date";

export default function App() {
  return (
    <Provider theme={defaultTheme} height="100%">
      <Button variant={"primary"} onClick={() => {
        parseDuration("PT36H")
      }}>Click Me</Button>
    </Provider>
  );
}

Version

3.32.0

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

If other, please specify.

No response

What operating system are you using?

Linux

🧒 Your Company/Team

No response

πŸ•· Tracking Issue

No response

ktabors commented 5 months ago

Thanks for the identifying this issue. parseDuration() from @internationalized/date should match the behavior of Temporal.Duration.

mfwgenerics commented 5 months ago

Thanks @ktabors

For the benefit of future readers of this issue, here's the behavior of Temporal.Duration using the polyfill currently hosted on the reference docs.

const duration = Temporal.Duration.from("PT36H")
duration.hours
> 36
duration.days
> 0
limkhl commented 1 month ago

I've created a PR since it seems to be unresolved. https://github.com/adobe/react-spectrum/pull/7064 This is my first contribution to an open-source project, so please let me know if there's anything I've missed.