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

Locales? #36

Closed
aolko opened this issue Apr 24, 2018 · 34 comments
Closed

Locales? #36

aolko opened this issue Apr 24, 2018 · 34 comments
Labels
discussion Further discussion

Comments

@aolko
Copy link

aolko commented Apr 24, 2018

will there be any locales avaliable?

@iamkun
Copy link
Owner

iamkun commented Apr 24, 2018

update: check our doc here
https://day.js.org/docs/en/i18n/i18n






Yes.
But I am looking for a simple implementation, not to expand our library.
e.g.
import dayjs from 'dayjs';
dayjs.setWeekName(['dimanche', 'lundi', 'mardi', ...]) // French;
dayjs().format()// then return locales
How do you think?

@aolko
Copy link
Author

aolko commented Apr 24, 2018

sorry, i don't speak ES5/6 😅

maybe stick to a moment.js approach for now?

@iamkun
Copy link
Owner

iamkun commented Apr 24, 2018

😅 ok, here is the compiled one in browser

<script src="https://unpkg.com/dayjs"></script>
<script>
// French locales
dayjs.setWeekName(['dimanche', 'lundi', 'mardi', ...]) 
// return locales [dimanche]
dayjs().format('[dddd]') 
</script>

@aolko
Copy link
Author

aolko commented Apr 24, 2018

hmm, alright
how about "from now","ago","last week","last month",[...] localisations?

@iamkun
Copy link
Owner

iamkun commented Apr 24, 2018

This is much like moment().fromNow() API dose.

But do we really need an API like this?

I mean perhaps some one want to return a string "4 years", another one want "4 years ago", and another one want something else(number 4 maybe). And maybe it's hard to satisfied them all.

How do you think?

@iamkun iamkun added the discussion Further discussion label Apr 24, 2018
@b44rd
Copy link

b44rd commented Apr 24, 2018

How about going for something like this for handling language overrides? I think it looks pretty sweet.

// Only override weekdays and months
import { weekdays, months } from 'dayjs/locale/fr'
dayjs.setLocale({ weekdays, months })

Alternatively, for including an entire language file

// Override entire language
import * as locale from 'dayjs/locale/fr'
dayjs.setLocale(locale)

@mauron85
Copy link

mauron85 commented Apr 24, 2018

I just came here from hn. I already had proposal for date-fns to use Intl.DateTimeFormat for most cases, so I can only suggest same here. :) Good job so far.
date-fns/date-fns#464

@aolko
Copy link
Author

aolko commented Apr 25, 2018

and what are we going to do, for babelized js? You're going to use babel, right?

@Frondor
Copy link
Contributor

Frondor commented Apr 27, 2018

I like b44rd approach. Just pass an object of key-value strings and let dayjs initialize with it, by overriding the default strings with the provided ones.

@schiem
Copy link
Contributor

schiem commented Apr 27, 2018

I'll start working on a PR for the approach outlined by @b44rd , unless someone has an objection.

@Frondor
Copy link
Contributor

Frondor commented Apr 27, 2018

@schiem I was trying something already :D But maybe we can finish it together. Actually, I'm considering two approaches, since the library actually limit the instantiation to its own module, and that could lead to performance issues when applying new strings. I'll show my progress as soon as I get home, after work

@SBoudrias
Copy link

I think a big mistake moment did is making the configured locale global.

This is a big issue for Node.js server side where many requests might be processed at the same time.

Having a API returning a new dayjs instance with the localization would be better:

import dayjs from 'dayjs';
import withLocale from 'dayjs/locale';
import frenchConf from 'dayjs/locale/fr-FR';

const dayjsFR = withLocale(dayjs, { weekdays: ['Dimanche', 'Lundi', ...etc] });
const dayjsFR2 = withLocale(dayjs, frenchConf);

@Frondor
Copy link
Contributor

Frondor commented Apr 28, 2018

@SBoudrias you could store different instances per language with this approach #77

const dayjsFR = dayjs(null, { WD: ['Dimanche', 'Lundi', ...etc] });

Pinging @schiem 😃

@schiem
Copy link
Contributor

schiem commented Apr 28, 2018

That looks good to me! That's really similar to the approach I was thinking of.

I think if we add localization, we'll need to rethink how abbreviated months are done, since it currently just takes the first the first three letters of the month, which isn't in line with how other languages handle the abbreviation. My guess is that the only fully localized way to handle that will be with a separate array of abbreviations.

Apart from that consideration, the only other thing I can think of is adding a few more tests to make sure that the language specific set/get are working (tests for calling dayjs(...).set('minuto', 10)).

@Frondor
Copy link
Contributor

Frondor commented Apr 28, 2018

I don't think how months are being abbreviated is a problem. I'm pretty sure the first three letters convention is world-wide accepted. I'd be worried about pluralization instead.

In the future, for newer formats, we may need some "internationalized" helper for pluralization. Since, for instance, the singular for months is month (without last s), but in spanish, meses (which means months) is just mes (without last es). So a pluralizer() helper wouldn't work by just adding an s to every term passed through it ⚠️

Apart from that consideration, the only other thing I can think of is adding a few more tests to make sure that the language specific set/get are working (tests for calling dayjs(...).set('minuto', 10)).

In regards to that, we must discuss if set is using the localized word or not. If you ask me, I vote to keep it in english, so the whole source code still operates using the english conventions.
Then you internationalize your dayjs instances, but still writing code like dayjs(...).set('minutes', 10) (it also helps to follow documentation examples, and avoid issues like "hey this code doesn't work..., Oh, it's because I have to use the localized term instead, my bad")

@iamkun
Copy link
Owner

iamkun commented Apr 28, 2018

@Frondor Looks Nice.

I just wondering if it's better returning a new function instead of a new instance.

import dayjs from 'dayjs'
import localeES from 'dayjs/locales/es'

const spanishDayjs = dayjs.locale(localeES)
// then just use spanishDayjs later

spanishDayjs().set(...).format(...)

@schiem
Copy link
Contributor

schiem commented Apr 28, 2018

I agree that it makes sense to keep that portion in English. Unless I'm reading the PR incorrectly (I may be, correct me if I am), the changes at index.js:137-156 will change the set function to require the localized language to be used (the switch statement will go from case 'minute': to case 'minuto':, since the constant this.$C.MIN is being changed with the localization).

Would a pluralizer function that can be overridden in the localization work (passed as a piece in the locale config)? So for English the default function would return the input string + 's', but for other languages you could handle the edge cases directly in the function with if tests. The trade off would be that the library would stay super small since the English function could just be one line, but the pluralizer function might be unwieldy in other languages.

@Frondor
Copy link
Contributor

Frondor commented Apr 28, 2018

I just wondering if it's better returning a new function instead of a new instance.

if dayjs.locale static method is initializing the class itself under the hood, personally, I see no difference or advantage.
It could be dayjs(...).setLocale(...).format(...) as well, or even both ways since the only thing we need is to apply the locales to this.$L instance property. It's up to you.
I'm just not sure about applying the locales on every dayjs() call, I think we should worry about performance on this one, and provide a way to create date objects out of the same dayjs instance (which holds the already configured default language, plus more -upcoming- configuration)

Unless I'm reading the PR incorrectly ... the changes at index.js:137-156 will change the set function to require the localized language to be used

I think you may be looking at the first commit. I committed a mistake, but latter switched back to C constants for the conditional statements https://github.com/Frondor/dayjs/blob/1f3c5f3e307d0713bfbbbe5a43ffba817266f39f/src/index.js#L139

Would a pluralizer function that can be overridden in the localization work

Pretty much, it can even be shipped within src/locales/{lang}.js file, which is loaded on demand. For the UMD version, a new bundle may be created from rollup for each of those files.

@schiem
Copy link
Contributor

schiem commented Apr 28, 2018

Ah, you're right, I was looking at the wrong commit, my bad. Looks good to me!

@iamkun
Copy link
Owner

iamkun commented Apr 28, 2018

@Frondor Another idea here. We might better give our user a choice whether to set local in current line of code, a new instance, or set global.

A pseudo code here:

let local = 'Monday'
class Dayjs {
  constructor(config, local) {
    this.local = null
    if (local) this.local = local
  }

  setGlobalLocal(l) {
    local = l
  }

  format() {
    return this.local || local
  }
}
const dayjs = (config, local) => (new Dayjs(config, local))

dayjs.newLocalDayjs = (local) => (config) => (new Dayjs(config, local))

export default dayjs 

And usage:

import { spanishDay, FrenchDay } from 'dayjs/locales'
// 1
dayjs() // normal dayjs
// 2
dayjs(20180808, spanishDay) // set local in this line only
dayjs() // still normal dayjs
// 3
dayjs.setGlobalLocal(spanishDay) // dayjs turns to spanish 
dayjs() // spanish dayjs
// 4
const FrenchDayjs = dayjs.newLocalDayjs(FrenchDay) // get a new FrenchDayjs function
FrenchDayjs() // french dayjs
dayjs() // still normal dayjs

@schiem @SBoudrias opinions?

@Frondor
Copy link
Contributor

Frondor commented Apr 28, 2018

I don't think we should encourage users to set different locales per instance. Remember every time you provide locales you're doing Object.assign computations.
Anyway, can you think of a real world example where someone needs to provide formatted dates for different languages from the same instance, probably in the same page? If so, and it turns to be useful, then I suggest a second parameter in dayjs(...).add(...).format('...', overrides).

At this moment (in my PR), the whole cycle lets you add / switch / override locales three times: dayjs(20180808, {locale: frFR}).setLocale(enGB).format('...', esES)

I know it's out of the scope of this issue (probably should be addressed by another PR), but I'll show an "advanced" use case for this lib; we've building a lightweight ORM and my goal is to provide date casting with dayjs. I can share an example of how it works with localization.
But for this to work, I want to suggest a change on the way how dayjs actually instantiates itself. So a default format can be provided for JSON.stringify(dayjsObject) to use through dayjs.toJSON method (which should do this.format() instead of this.toISOString()).

You may ask why not dayjs().defaultFormat('...')?, well... In my humble opinion, I consider the method chain is actually pretty long and with many configurations incoming (as the project grows), you'll see your how code ends up looking better as dayjs(initConfig) instead of dayjs().setXconfig().setYconfig().setZconfig().... But once again, it's my opinion, and the use case I'm showing as example is just to help me explain my point.

We can easily solve that by doing something like this:

The class is exported for the user to manually instantiate; let's say import dayjs from 'dayjs' imports an instance, and import { Dayjs } from 'dayjs' imports the class.

import { Dayjs }, dayjs from 'dayjs'

const FrenchDayjs= Dayjs({ format: 'YYYY', locale: frFR, date: nullValue || new Date() })
// or
const date = dayjs(new Date())

console.log(JSON.stringify(Dayjs)) // prints "2018" because it internally uses obj.toJSON() which uses default format provided instead of current toISOString()

You can see how this kind of use case benefits from the new config here (note the MockDayjs object):
https://codepen.io/Frondor/pen/BxpLZG?editors=0012
(It's tedious, but easy to follow the way how I use dayjs to provide date casting)

class Car extends Model {
  get dates () {
    return {
      created_at: 'dddd of YYYY',
      updated_at: ''
    }
  }
}

const car = new Car({
  created_at: new Date(2018, 3, 28),
  updated_at: new Date(2018, 3, 29)
})

const year = car.created_at.format('YYYY') // "2018"

const serialize = JSON.stringify(car) // '{created_at: "Saturday of 2018"}'

TL;DR

What I suggest is a set of (backwards compatible) changes in favor of performance and flexibility

  • provide exports for Dayjs class, while keeping the default singleton dayjs
    i.e.: import { Dayjs }, dayjs from 'dayjs'
  • Dayjs class constructor receives a config object, instead of Date or any of the date conventions. dayjs singleton still works as intended, but under the hood it now does
    export default (date, config) => new Dayjs({ ...config, date: parseConfig(date) })
  • dayjs.toJSON to just call this.format() and get rid of all the ISOString conversions by using the default format (could be overriden by config.format)

If some of you is unsure about what I mean (my english sucks, I know), I can create another branch to display the changes, just ask for that 😉

@iamkun
Copy link
Owner

iamkun commented Apr 29, 2018

Remember every time you provide locales you're doing Object.assign computations.

We might could just use direct assignment instead of Object.assign. Besides, Object.assign is an
API in ES6, that is to say, if we want to support old browsers, we have to include a polyfill for it, which is a lot of code.

Anyway, can you think of a real world example where someone needs to provide formatted dates for different languages from the same instance, probably in the same page?

As far as I know, a nodejs server shares the same global environment for every incoming request. According to @SBoudrias' note, changing the global moment might cause a bug.

@iamkun
Copy link
Owner

iamkun commented Apr 29, 2018

Still, I'm afraid I didn't get what you mean. Sorry. May be I have to go through your code and comment again tomorrow.

Questions,

  • 1 why we are going to change dayjs#toJSON ?
  • 2 what's this ORM here used for? (Confused me most)

Plus, any direct opinion or suggestion to my pseudo code above? Looking for a feed back. :D

Good job so far. I think we are close to the FINAL locales we are looking for.

@Frondor
Copy link
Contributor

Frondor commented Apr 29, 2018

I knew the ORM example would only bring more confusion to my horrible English 😅 I'll try to be more compact this time.

why we are going to change dayjs#toJSON ?

Because JSON.stringify always look for a toJSON method on nested objects at serialization time. If you provide it, then every time you do JSON.stringify on an object that contains said method (a dayjs instance), it shall use it instead of its own (if you don't provide it, I think it will expose all your "fake" private properties like $d as a string). But I'm pretty sure you already knew that, so the point is; if you do user = { created_at: dayjs(new Date()) } and then JSON.stringify(user), there's no way you can get something like {created_at: "29 / 08 / 2018"}, because it's using this.toISOString() which generates this string: '{created_at: "2018-04-29T19:49:59.347Z"}'.
Now imagine you have a collection of those User, and you need to stringify it. You could do something like

userCollection.map(user => {
    user.created_at.format()
    return user
}

I'm afraid this is a caveat in moment.js as well, but to be honest, I find unnecessarily redundant to have a toJSON method doing the same as toISOString on a date object.
That's why I suggest letting the user provide a default format, and let toJSON() and toString() invoke this.format() instead. It may be relevant to #73, and probably its the way to fix the primitive conversion between browsers.

what's this ORM here used for? (Confused me most)

Added some text in the HTML panel of that codepen for you, so I don't bloat this comment (pay a look to MockDayjs class and the toJSON method, which makes possible the serialization you see in the console)

We might could just use direct assignment instead of Object.assign. Besides, Object.assign is an
API in ES6, that is to say, if we want to support old browsers, we have to include a polyfill for it, which is a lot of code.

I agree! We probably need to move the default language constants from src/constants to src/locales/en, and do direct assignment.

Anyway, can you think of a real world example where someone needs to provide formatted dates for different languages from the same instance, probably in the same page?

As far as I know, a nodejs server shares the same global environment for every incoming request.
According to @SBoudrias' note, changing the global moment might cause a bug.

I'm not sure if this solves the issue you're referring to, but personally, in node, I'd always try to avoid loading locales per-request. I'd rather create a moment instance for each supported language and inject the correct instance through a service module depending on req.headers["accept-language"].

@Frondor
Copy link
Contributor

Frondor commented Apr 29, 2018

Plus, any direct opinion or suggestion to my pseudo code above? Looking for a feed back. :D

Looks similar to my approach, but the newLocalDayjs approach adds a bit of unnecessary complexity in my humble opinion. Also, if you pass a string to dayjs(20180808, 'spanishDay') , that means you'll be requiring locale objects dynamically, and the UMD version is going to ship with every locale we support, that's going to increase the size a lot and even moment.js doesn't do that. Or probably you meant an object and not a string? Anyway, that's why I suggest to simply

export class Dayjs { constructor (config) {...} }
export default const dayjs = (date, config) => new Dayjs(config) } // config.date = date

and let the user handle the instance however he wants (see my MockDayjs example)
EDIT: Just updated the my local dev branch with this suggestion, but things can be reversed if needed ;)

@iamkun
Copy link
Owner

iamkun commented Apr 30, 2018

and the UMD version is going to ship with every locale we support

My mistake. It's an imported object not a string. I've corrected the code above.

The class is exported for the user to manually

Export Dayjs class is a good idea. It ships more flexibility.

Plus, I think there's no doubt that we should add locales and plugins to dayjs at the moment. And I want to discuss the API design. We might keep the tradition style :

dayjs.local(French)
dayjs.extend(AdvancedFormat)

or just one api for less confusion. In this way, a locale is also a kind of plugin:

import { LocaleFrench, AdvancedFormat } from 'dayjs/plugin'
dayjs.use(LocaleFrench)
dayjs.use(AdvancedFormat)

How do you think.

@Frondor
Copy link
Contributor

Frondor commented Apr 30, 2018

I agree with both ideas. At the moment, we don't have dayjs.local but dayjs.setLocale and dayjs.format(..., { locale: FR }). It's a matter of taste I think, I don't really mind on which term to use for this method.

You can also do dayjs(new Date(), { locale: FR })

or just one api for less confusion. In this way, a locale is also a kind of plugin:

Now that I think, I prefer your first example with dayjs.extend((instance) => instance.$xyz = 123), it's super easy to work with and all it does is to "pipe" a callback with access to the dayjs instance for the user to mutate it. But that's probably out of the scope for localization :D

@iamkun
Copy link
Owner

iamkun commented May 1, 2018

OK, here comes a problem. How can we set a global locale config?
It might because of the limitation of the structure in our current code.
We can do dayjs().setLocale(esES) on a instance, but we can't do dayjs.setLocale(esES) globally. Isn't it?

Any suggestion about a global configuration?

dayjs.local(esES)
dayjs.format() // esES by default
dayjs.format() // esES

@iamkun
Copy link
Owner

iamkun commented May 2, 2018

Plus, @Frondor @schiem would you mind making a new pull request to branch @next, instead of direct to master.

We are currently implementing two important features locales as well as plugin system. And our code might dependence others.

I want get everything(test, docs, rollup config...) done before merge to our main branch.

Thanks

@schiem
Copy link
Contributor

schiem commented May 2, 2018

@iamkun Absolutely. Would you prefer a new PR or just modifying the existing one?

@Frondor
Copy link
Contributor

Frondor commented May 2, 2018

Alright, we can't change the target branch, so I have to close this PR and send a new one to the new branch, it actually targets dev.

Anyway, what's the plugin system? Can't we tackle it by just creating an extend method that receives a callback with this passed to it? i.e.: extend (fn) { fn(this); return this } or even extend (fn) { fn.call(this); return this }

OK, here comes a problem. How can we set a global locale config?

Actually, I think there's not way we can do that without using globals or env variables (I recommend none of both). I suggest to encourage the user to create base instances with its own config, and then set() / clone() it

I've written a full example of what's done so far in my PR, #77 and new approachs to some methods

@schiem
Copy link
Contributor

schiem commented May 2, 2018

@Frondor The original intent behind the plugins was to also allow overwriting built in methods, e.g. allowing more format options without needing to add bloat to the core. So you could still call dayjs().format("some arcane format option") instead of dayjs().pluginFormat("..."). And since it takes the place of the original method, it has access to this by default. I'm not 100% sure that this is the best approach, I'm planning on giving it another look when I get home to see if I can come up with something more elegant.

OK, here comes a problem. How can we set a global locale config?

The way that I handled that for the plugin system was by having an instance set it on the prototype, like:

function extend(plugin) {
    const proto = Object.getPrototypeOf(this)
    proto[plugin.name] = plugin.method
}

Could overwriting a property of the prototype be done to set locales?

@Frondor
Copy link
Contributor

Frondor commented May 2, 2018

@schiem you can take a look to the Vue approach to plugins, you may extract some ideas from there. I personally like how the install method just receives the Vue object and you add / overrides to its prototype. Then it does some work internally depending on what you registered (for example, merging the mixins and creating the directive objects, but that's not really needed here)

Could overwriting a property of the prototype be done to set locales?

Can't see the necessity for that though, the object locales is always living in the $L property. Please, see how it works here #90 Every review is welcome

@iamkun
Copy link
Owner

iamkun commented May 22, 2018

Locales released in 1.6.0
Closing this due to inactivity. Feel free to re-open it any time.

@iamkun iamkun closed this as completed May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Further discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@mauron85 @aolko @SBoudrias @schiem @Frondor @b44rd @iamkun and others