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

feat(date): add durationToString function to stringify durations as ISO 8061 strings #7788

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

Conversation

cyyynthia
Copy link
Contributor

@cyyynthia cyyynthia commented Feb 19, 2025

This PR introduces a new helper called durationToString to stringify DateTimeDuration objects to an ISO 8061 duration string.

Additionally, this PR makes parseDuration accept decimal values for all time scale components rather than just ones after the time designator. This is consistent with the ISO 8601-1:2019 specification and should improve interoperability. Tests that were incorrectly checking failure of parsing have been updated.

Side note: currently the parseDuration function wrongly accepts strings containing the week time scale component mixed with other components. My interpretation of the standard is that this is incorrect; a duration string can either be composed of year, month, day, hours, minutes, seconds (optionally omitting zero values), or PxW. This isn't a major issue imho, but I've made sure that the library doesn't produce such invalid strings.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Simply experiment with the durationToString function. Tests have been added to the test suite, and documentation have been updated, which contain examples of how to use the function.

…ngify durations as ISO 8061 strings

Additionally, this commit makes `parseDuration` accept decimal values for all time scale components rather than just ones after the time designator.
This is consistent with the ISO 8601-1:2019 specification and should improve interoperability.
@cyyynthia
Copy link
Contributor Author

I'm unsure about the check nagging me about the PR title; it seems the regex is quite a bit restrictive and not fond of the slash, which imho is correct here 😅

@snowystinger snowystinger changed the title feat(internationalized/date): add durationToString function to stringify durations as ISO 8061 strings feat(date): add durationToString function to stringify durations as ISO 8061 strings Feb 20, 2025
@snowystinger
Copy link
Member

snowystinger commented Feb 20, 2025

the pr title linting is a work in progress and nothing is hooked up to the name category yet, I have changed it to just 'date' as that is more than fine for what we use it for
thanks for pointing out the more correct naming

also, we are mid release cycle right now, so apologies if there is a bit of a delay for looking at this pr

@snowystinger
Copy link
Member

Thanks for the PR. We had a chance to discuss this today as a team. We'd like to propose a couple of changes. Instead of this function on its own, we'd like for parseDuration to return a class which has a toString function implementing this logic.

The class should match TemporalDuration
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Temporal/Duration
As Temporal is starting to ship and it'd be good if we made that future transition easier.

@cyyynthia
Copy link
Contributor Author

That's understandable, although I wonder if that wouldn't incur breaking changes 🤔

Besides, sounds great, would make manipulating durations a lot easier (immutably that is); just worried a bit by the breaking nature of using a class compared to a plain object.

@snowystinger
Copy link
Member

So long as the class implements the same properties as the object, we should be fine. The types should still match as well. Was there another worry for breaking changes there?

@cyyynthia
Copy link
Contributor Author

Immutability isn't something that is currently required to deal with, it's possible to mutate plain objects currently but would require using appropriate methods using a class. Also, Temporal objects implement toJSON, which would change the behavior of serializing this type of object

@snowystinger
Copy link
Member

I think the change to toJson could be considered a bug fix as we're meant to be matching Temporal when we can. I don't know how common it is for people to call toJson on the object we currently return from parseDuration. If we're worried about it, we could allow an argument to have it do the old behaviour.

Immutability is a little more concerning to me since that one can pop up in unexpected places.

We could consider that parseDuration take a new arg which has it return the Temporal aligned class instance so that existing usage doesn't change. I'll bring it up with the team.

@cyyynthia
Copy link
Contributor Author

I don't know how common it is for people to call toJson on the object we currently return from parseDuration

The biggest problem is that toJSON is automatically called by JSON.stringify, which is very handy but means the object would no longer be serialized as an object, but as the ISO string

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