Skip to content
This repository has been archived by the owner on Jul 10, 2019. It is now read-only.

Feature request: Allow a custom value for now() #26

Closed
jbaudanza opened this issue Mar 2, 2015 · 12 comments
Closed

Feature request: Allow a custom value for now() #26

jbaudanza opened this issue Mar 2, 2015 · 12 comments
Assignees

Comments

@jbaudanza
Copy link
Contributor

Here are some possible solutions:

  1. Change the signature of format to be format(date, now). now would be optional.
  2. Add a now option to the constructor. This could be either a Date/number constant or a function reference.

My motivation for this request is to use intl-relativeformat from within a React component. I can use this from within React now, but idiomatic React components are supposed to be pure functions of this.props and this.state. Having my render function call Date.now is somewhat of an antipattern.

Let me know what you think. I'd be happy to put together a PR.

@caridy
Copy link
Collaborator

caridy commented Mar 2, 2015

@jbaudanza I understand, but the problem is the CLDR data that we use, which only account for relative to now. Think about this, it is not the same saying a week ago then a week before, which is what we will have to use if we want to support a custom point in time to calculate a relative value to that point in time. Unfortunately we don't have that data, we will have to look into CLDR project to see if that value can be constructed, and if it does, maybe we can add a new API, since the current API implies NOW as a computed value, not a provided value.

@jbaudanza
Copy link
Contributor Author

I should clarify. I'm not suggesting that the format function should describe time duration relative to anything other than "now". I just want to change the mechanics of how the library gets the value of "now". The semantics of what "now" means should remain the same.

The CLDR output should remain the same.

Here are a couple contrived examples to illustrate my point:

  1. I want to calculate many relative time strings, and I only want to call Date.now() once at the beginning for some performance reason.
  2. I am reviewing an error report that was received last week that contains the complete application state at the time of the error. (This is very possible with React). I would like to be able to reproduce the state of the DOM using this state information, which involves passing a stale value of "now" into the format function.

Let me know if this makes more sense.

@caridy
Copy link
Collaborator

caridy commented Mar 2, 2015

ok, I understand now.

For 1), I don't think Date.now() will be the perf bottleneck since it is very optimized as today. We are talking about +15M ops/sec on the latest chrome. I will like to know more about those perf reasons you mentioned.

As for 2), that's a genuine request, we will look at it, but it is a hard to sell IMO.

@jbaudanza
Copy link
Contributor Author

There aren't any real perf reasons. That was a contrived scenario to help clarify my request.

My original motivation was to keep my react components pure functions.

@caridy
Copy link
Collaborator

caridy commented Mar 2, 2015

Good, that's what I thought.

To clarify more on why is this going to be a problem, is that we plan to add relative time support into ECMA-402, specifically in Intl.DateTimeFormat constructor, and that one will take care of the now() under the hood for sure, and this package will become just a polyfill of the standard specification.

@ericf
Copy link
Collaborator

ericf commented Mar 6, 2015

We had a similar issue to the error report example when writing functional tests for the FormatJS website. The hack we used to address this is override Date.now() in the test environment to return a static value. It seems someone could use that hack in an error reporter.

As for the pure functions, I see the point, but relative time is in fact temporal. A wall clock UI implemented in React would have a similar issue at some point within the component hierarchy. Could you elaborate more on your pure function motivation?

@jbaudanza
Copy link
Contributor Author

I don't have any motivation other than the standard motivations for writing pure functions. They are generally easier to reason about and are the idiomatic way to write React components. React also has some optimization tricks that are only available if your component's render function is pure.

I understand this seems like kind of an edge case. But, it does seem that the library is unnecessarily coupling a pure function that calculates string with a system call to fetch the current time. Your monkey-patching test is another great example of why these two pieces of functionality shouldn't be tightly coupled.

As is the case with libraries, I suspect there will be other use cases down the road that run into friction due to this coupling.

For what it's worth, twitter seemed to go in the pure direction with their relative time module. This is from the twitter-cldr-js README:

var fmt = new TwitterCldr.TimespanFormatter();
var then = Math.round(new Date(2012, 1, 1, 12, 0, 0).getTime() / 1000);
var now = Math.round(Date.now() / 1000);

fmt.format(then - now);                    // "6 months ago"

I imagine a React wall clock component would periodically fetch the current time and store it in a this.state or perhaps in a root this.context variable. The render function would then remain pure.

@ericf
Copy link
Collaborator

ericf commented Mar 6, 2015

So accepting a diff would be the way to do this for sure. So we can look into if/how we might do this. It's likely to be a backwards incompatible change at this level, but we could provide both ways of providing data at the React level.

@ericf
Copy link
Collaborator

ericf commented Mar 8, 2015

  1. Change the signature of format to be format(date, now). now would be optional.

I'm leaning towards a modified version of this suggesting. I think it will be confusing as to which order these arguments go in, so I think adding an options argument would be the way to go:

var relativeFormat = new IntlRelativeFormat('en');
relativeFormat.format(post.date, {now: Date.now()});

options.now is inherently optional, and the value to represent "now" will be labeled now, making it distinguishable from the required date/time value.

The reason I think this is better than adding a constructor option is that the style and units constructor options apply no matter what values your formatting. Whereas, the format() prototype method is saying: format this date relative to now, so optionally being able to specify a now value seems appropriate since these are the two dynamic values in the context of the format() invocation.

@ericf ericf self-assigned this Mar 8, 2015
@ericf ericf removed the question label Mar 8, 2015
@jbaudanza
Copy link
Contributor Author

Yup, I agree. I'll throw together a PR.

@ericf
Copy link
Collaborator

ericf commented Mar 9, 2015

Closing now that #27 was merged.

@ericf ericf closed this as completed Mar 9, 2015
@ericf
Copy link
Collaborator

ericf commented Mar 13, 2015

formatjs/formatjs#94 adds a now prop to the <FormattedRelative> component in React Intl.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants