dayjs.locale() throws type error due to type update in v.1.8.30

Created on 22 Jul 2020 · 11Comments · Source: iamkun/dayjs

Describe the bug
The locale() function used to accept a string up until v1.8.30, which updated the types to use LocalePresetType (https://github.com/iamkun/dayjs/commit/73813ab5851c118450f5d8fc15cae71d49d800ae). This has broken our CI/CD pipelines, as our i18n implementation returns the locale code as a string.

Expected behavior
Patch versions should not include type changes that are not backwards compatible with previous versions.

Information

Day.js Version [1.8.30]
OS: [Ubuntu 16.04]
Browser [n/a]

suggestion
Add back string as a valid type either in the function declarations or in the LocalePresetType union.

We can work around this issue by casting the string to a LocalePresetType but we would prefer if we didn't have to do this everywhere our i18n interacts with dayjs.

released ☢️Bug

Source

B1rch

👍5

Most helpful comment

:tada: This issue has been resolved in version 1.8.31 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

iamkun on 29 Jul 2020

👍2 ❤1

All 11 comments

Sorry for the inconvenience. We will fix this by adding a back string type in LocalePresetType in 1.8.31 shortly.

iamkun on 22 Jul 2020

👍2

One problem is if I added string as a backup type. It will be useless to add these preset strings?

e.g.

type localeName = 'en' | 'es' | string

Typescript will treat and string as valid, and IDE will have no auto-complete at all.

iamkun on 22 Jul 2020

That is true, and there might be better way to achieve this. But I don't imagine people are hardcoding locale strings all to often. Regardless, not having breaking changes in a patch version has a higher priority over QOL changes if it were up to me.

I haven't looked at the source so I don't know what the previous behavior was, but maybe you can look into adding a warning if the locale code provided to the function is not supported, and perhaps adding the option for a fallback locale to be used in that case.

B1rch on 22 Jul 2020

👍1

Maybe we should roll back to string only instead of the listed locale names.

iamkun on 22 Jul 2020

👍2

Make it a major release and mark it a breaking change would also do it. I prefer stricter typing.

muuvmuuv on 23 Jul 2020

👍1

@muuvmuuv It just comes to me that there might be user-customized locale name, like 'aaa', maybe the only thing we could do is fall back to 'string' type.

iamkun on 24 Jul 2020

I don't think so because you can extend the type with custom types like this:

type Locale<AdditionalTypes = void> = LocalePresetType | AdditionalTypes

type CustomLocales = 'aaa' | 'bbb'

const one: Locale = 'az'
const two: Locale<CustomLocales> = 'aaa'

I would fall back to string only if dayjs locale gets intelligent enough so that de-DE gets recognized as de, then typings are not longer needed. Or at least append string so the build in locales are typed. Momentjs does it that way like I mentioned in another issue.

muuvmuuv on 24 Jul 2020

👍1

I'm sorry that I suggested this type. https://github.com/iamkun/dayjs/pull/955
It caused the unpredictable inconvenience.

@muuvmuuv 's suggestion looks good to me in the long run.
But in near future, should add string to LocalePresetType as a interim solution, I think.