Skip to content

Return environment variable as a date object #13

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ledleds
Copy link

@ledleds ledleds commented Apr 9, 2019

Thanks for this library, we find it very useful!

We sometimes have dates as environment variables and have been checking that when a date is given that it is valid everywhere we use your library. I thought maybe others could be doing this also so it might be useful to be within the library itself!

The new date function uses another library dayjs for date validation and returns a date object with the given value.

Please let me know what you think!

@ctavan
Copy link
Owner

ctavan commented Apr 9, 2019

Hi @ledleds, glad to hear you find this library useful!

I like the idea of being able to handle dates more easily. And I also think the code quality of your patchset is really good!

However I'm a bit reluctant with respect to adding a dependency (dayjs) which is much more complex than this tiny library on its own just for the sake of being able to parse/validate dates which will add.

Do you see any chance of extracting the logic behind dayjs().isValid() and inline it directly in this library here?

url: url.parse
url: url.parse,
date: function(value) {
if (dayjs(value).isValid()) {
Copy link
Owner

Choose a reason for hiding this comment

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

It would be great if we could find a way to do this without adding a comparably large dependency like dayjs (dayjs is much bigger than this library so it feels odd to add it as a first and only dependency).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah makes total sense. I'll look at how the dayjs library does the validation and see if its a simple replication or find some other way of validating!

@jshjohnson
Copy link

Swapping Moment out for Dayjs may make sense. Dayjs has the same interface as Moment but is only 2kb: https://github.com/iamkun/dayjs

@ctavan
Copy link
Owner

ctavan commented Sep 6, 2019

I still would love to keep this basic module free of external dependencies
. If you could inline the relevant functionality I‘d be happy to merge.

@ctavan
Copy link
Owner

ctavan commented Oct 9, 2019

@ledleds @jshjohnson any interest from your end of picking this up again and providing an implementation that works without adding dependencies?

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