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

#112 Get rid of moment over date-fns #116

Closed

Conversation

cristijora
Copy link
Collaborator

@cristijora cristijora commented Feb 13, 2017

I would like to present an option over moment.js which is date-fns.
Date-fns is very modular and you can require only the things you need from it. The downside though is that it doesn't support a variety of date formats. See skipped test. Might support them with version 2.0 which should come soon I think.
On the other hand now the build reduces to 80kb :)

Use babel-plugin-lodash to support "tree-shaking" for lodash
@@ -105,7 +105,8 @@ describe("fieldDateTimePicker.vue", function() {
});
});

it("model value should be the formatted input value if changed", (done) => {
//TODO These kinds of formats don't work with date-fns library
it.skip("model value should be the formatted input value if changed", (done) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why skipped? I see it support formatting.

Copy link
Collaborator Author

@cristijora cristijora Feb 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look at it tomorrow. I had parsing errors on this test but the library supports those formats.
https://jsfiddle.net/z11fe07p/729/
When running tests same statement as the one from the fiddle gives
LOG: Invalid Date

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fiddle is working at me. No error, in result: "2015-01-01T23:00:00.000Z"

Copy link
Collaborator Author

@cristijora cristijora Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know. Fiddle and webpack bin working, but as long as the same thing is used in the code it doesn't work. console.log(parse("2015.01.02")) outputs Invalid Date in the code although it works in the fiddle. Very strange. If we pass this error, we are good to get rid of moment js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird. I thought parse has a format parameter at least.

@icebob
Copy link
Member

icebob commented Feb 14, 2017

From momentjs we need only the format and parse functions. So we can search other date library only for this functions. I found it: https://github.com/taylorhakes/fecha
It would be good to try it. It can only this two functions and the size is ~2kB.

@cristijora
Copy link
Collaborator Author

Also isValid, isAfter, isBefore :)

@icebob
Copy link
Member

icebob commented Feb 14, 2017

OK, but these only for validation. And isAfter, isBefore is easy with unix epoch dates. I rewrite date validator if this lib take out momentjs.

@cristijora
Copy link
Collaborator Author

I will give it a try today. Seems strange though that date-fns format doesn't work here but works in fiddle

@icebob
Copy link
Member

icebob commented Feb 14, 2017

OK, thanks.

@cristijora cristijora mentioned this pull request Feb 14, 2017
@icebob
Copy link
Member

icebob commented Feb 14, 2017

Could you test fecha? Because if not, now I have time to try it.

@cristijora
Copy link
Collaborator Author

Please test it if you have the time. I can do it only in 4-5 hours or so.

@icebob
Copy link
Member

icebob commented Feb 14, 2017

Ok, I do it.

@icebob
Copy link
Member

icebob commented Feb 14, 2017

I tried, and it is working properly: #117
Bundle is 77k

@lionel-bijaoui
Copy link
Member

So date-fns or fecha ?

@icebob
Copy link
Member

icebob commented Feb 14, 2017

In this case fecha, because date-fns parse currently not supported format param, but there is format property in schemas

@lionel-bijaoui
Copy link
Member

Ok cool, when will you merge ?

@icebob
Copy link
Member

icebob commented Feb 14, 2017

Btw, pikaday & datetimepicker fields is not working in dev example. But it's not came with fecha, because not working in master branch too. I try to fix it.

@icebob
Copy link
Member

icebob commented Feb 14, 2017

@cristijora thanks this PR, but I close it. I copied your babel lodash solution to #117.

@icebob icebob closed this Feb 14, 2017
@cristijora cristijora deleted the improve-bundle-size branch March 5, 2017 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants