poppinss / indicative

Indicative is a simple yet powerful data validator for Node.js and browsers. It makes it so simple to write async validations on nested set of data.
https://indicative.adonisjs.com/
MIT License
417 stars 52 forks source link

moment.js requires too many locales #126

Closed wxs77577 closed 6 years ago

wxs77577 commented 7 years ago

moment.js requires all locales, is there any way to decrease the size?

Andreyco commented 7 years ago

I think, it's even possible to get rid off moment completely. Looking at existing codebase, it's only purpose is to format date, twice.

thetutlage commented 7 years ago

Yup, lemme see if I can do that

Andreyco commented 7 years ago

I suggest date-fns package. Has very small footprint, when importing per function, e.g.

import format from 'date-fns/format';

Also, would you consider bundling for browser usage? If so, I will try to reduce bundle size for better browser integration. On server, this is no brainer...

thetutlage commented 7 years ago

I don't think that anything will stop us from using it on browser. If there are any node specific dependencies, we can get rid of them.

Andreyco commented 7 years ago

No node specific dependencies there :) I do use it in browser. But I think bundle size could be reduced so that package leaves smaller footprint. I understand on server this is no issue at all.

thetutlage commented 7 years ago

Yes we can start by removing moment first and then work to reduce the bundle size. I am in for that

Andreyco commented 7 years ago

Awesome. Will look into this in next days, since I am about to use indicative in next project.

thetutlage commented 7 years ago

Closing, please create a PR when you start working

thetutlage commented 7 years ago

I pushed some changes to make indicative bit learner. I have removed dependency from inflect and Q. Kept moment for now, but happy if anyone wants to submit a PR that removes momentjs too

Andreyco commented 7 years ago

I moved everything from moment to date-fns, expect dateFormat validator which accept locale param.

https://github.com/Andreyco/indicative/commit/ee7e710e8827e07922ec970bf07298471ed4c0a4

To work with locales in date-fns, you have to explicitly require desired locale.

const distanceInWords = require('date-fns/distance_in_words')
const eoLocale = require('date-fns/locale/eo')

const result = distanceInWords(
  new Date(2016, 7, 1),
  new Date(2015, 0, 1),
  {locale: eoLocale} // Pass the locale as an option
)

Sure, indicative could reexport locales provided by date-fns - this is no issue. Problem is breaking current public API of indicative, where locale param would not be string but locale object.

Open for such change?

Edit

Looking at public API for raw validators and schema rules, I see date format does not accept locale at all. Only accepted param is format. Perhaps, it's possible to get rid of locales completely.

Andreyco commented 7 years ago

Remove all moment implementation, replaced with date-fns https://github.com/Andreyco/indicative/commit/93729213e393f38609ed0618bddfcf54217555f8

"Unfortunately", under the hood, dateFormat function no longer accepts locale param, as it was not used nor documented at all.

Edit: Bundle size before moment removal: 1.1 MB Bundle size after moment removal: 638 kB

moment with 455.55kb -> date-fns with 53.13kb

@thetutlage @poppinss please, provide feedback on this change

Andreyco commented 7 years ago

Related PR here #141

Andreyco commented 7 years ago

Hello guys, possible to get response on this? Thank you.

thetutlage commented 7 years ago

Yes @Andreyco it's my bad, I missed it. Lemme go through the PR tonight

Andreyco commented 7 years ago

no problem at all. Thank you very much for attention.

Andreyco commented 6 years ago

Resolved via #141