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

Computeds do not respect service locale #135

Closed
jasonmit opened this issue Dec 31, 2015 · 6 comments
Closed

Computeds do not respect service locale #135

jasonmit opened this issue Dec 31, 2015 · 6 comments
Labels

Comments

@jasonmit
Copy link
Collaborator

Computed macros do not respect the service's locale, but that's probably not something we can easily solve unless we decide to inject moment into all common factory types.

What we can do, and probably should do, is support some way of specifying a locale. Below are two proposals.

format('createdOn', 'MM/DD/YYYY', {
  locale: 'moment.locale'
})

Or support computed composition

locale(moment('createdOn'), 'moment.locale')
@jasonmit jasonmit added the bug label Dec 31, 2015
@jasonmit jasonmit changed the title Computeds do not respect locale Computeds does not respect service locale Jan 1, 2016
@jasonmit jasonmit added bug and removed bug labels Jan 1, 2016
@jasonmit jasonmit changed the title Computeds does not respect service locale Computeds do not respect service locale Jan 1, 2016
@jasonmit
Copy link
Collaborator Author

jasonmit commented Jan 1, 2016

@stefanpenner I'm leaning towards the composition approach since it's more flexible and condusive to supporting the entire moment API:

//examples of composition

locale(moment('createdOn'), 'moment.locale')

calendar(moment('createdOn'), null, {
  sameDay: '[Today]',
  nextDay: '[Tomorrow]',
  nextWeek: 'dddd',
  lastDay: '[Yesterday]',
  lastWeek: '[Last] dddd',
  sameElse: 'DD/MM/YYYY'
})

diff(moment('createdOn'), new Date())

fromNow(moment('createdOn'))

toNow(moment('createdOn'))

duration(10, 'hours')

@stefanpenner
Copy link
Collaborator

locale(format(moment('createdOn'), 'MM/DD/YYYY'), 'moment.locale')

isn't the format also locale specific?

@stefanpenner
Copy link
Collaborator

diff(moment('createdOn'), new Date())

fromNow(moment('createdOn'))

toNow(moment('createdOn'))

duration(10, 'hours')

this looks good, but seems orthogonal to the locale issue right?

@stefanpenner
Copy link
Collaborator

Computed macros do not respect the service's locale, but that's probably not something we can easily solve unless we decide to inject moment into all common factory types.

I'm kinda fine with that, but likely only if the thing injected is unlikely to collide with peoples existing properties.

@jasonmit
Copy link
Collaborator Author

jasonmit commented Jan 2, 2016

Yes, bad example. You would not use format with locale.

@jasonmit
Copy link
Collaborator Author

jasonmit commented Jan 2, 2016

this looks good, but seems orthogonal to the locale issue right?

Not really. Since we would be adding locale support to all those computeds by fixing this issue, this means adding a tail argument or supporting an options hash. And the more I thought about it, the more it makes sense to just mimic the chaining behavior of moment API with our computeds.

locale(fromNow(moment('createdOn')), 'moment.locale')
locale(toNow(moment('createdOn')), 'moment.locale')
locale(duration(10, 'hours'), 'moment.locale')

or

format('createdOn', 'moment.locale')
fromNow('createdOn', 'moment.locale')
toNow('createdOn', 'moment.locale')
duration(10, 'hours', 'moment.locale')

The above approach is more or less what we have today, minus the tail argument for locale. It's problematic though since there are arguments in between moment and the locale that would need to be explicitly passed. So the above would really look like:

format('createdOn', 'MM/DD/YYYY', 'LLLL', 'moment.locale')
fromNow('createdOn', false, 'moment.locale')
toNow('createdOn', false, 'moment.locale')
duration(10, 'hours', 'moment.locale')

The advantages:

  • Familiar chaining behavior of moment
  • The computed callbacks implementation would result in something like moment[fn](...unwrappedArguments); reducing the complexity of the addon
  • Not having to implement locale support in each of the computeds explicitly
  • Not having to inject the moment service into every popular type factory avoiding your concern of prop key collision

The disadadvantages:

  • API breaking

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

No branches or pull requests

2 participants