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

Remove all functions that create the current date internally #377

Closed
leshakoss opened this issue Jan 6, 2017 · 2 comments
Closed

Remove all functions that create the current date internally #377

leshakoss opened this issue Jan 6, 2017 · 2 comments
Assignees
Labels
Milestone

Comments

@leshakoss
Copy link
Contributor

@leshakoss leshakoss commented Jan 6, 2017

Hello everyone! I've decided to delete a lot of functions in v2.0.0. Here is the list of them:

  • distanceInWordsToNow
  • isFuture
  • isPast
  • endOfToday
  • endOfTomorrow
  • endOfYesterday
  • startOfToday
  • startOfTomorrow
  • startOfYesterday
  • isToday
  • isTomorrow
  • isYesterday
  • isThisSecond
  • isThisMinute
  • isThisHour
  • isThisWeek
  • isThisISOWeek
  • isThisMonth
  • isThisQuarter
  • isThisYear
  • isThisISOYear

The common thing between all of them is that they all are shortcuts to other functions with one argument mapped to be the current date.

The problem with these functions is that they are not pure (i. e. they can produce different results with the same arguments in the same environment).

That creates a problem with the planned FP functions: #253.
Example:

import distanceInWordsToNowWithOptions from 'date-fns/fp/distanceInWordsToNowWithOptions'
const distanceInWordsToNow = distanceInWordsToNowWithOptions({addSuffix: true})

// days later:
const result = distanceInWordsToNow(new Date())

In this scenario, should "now" be defined when currying the first argument or the last one? The answer is, this function just doesn't makes sense in the context of FP since it produces the side effects.

Creating a new date isn't the only side effect of our functions. The other one is the user timezone - the reason why using the same function chain can produce the different results on the different computers. That's why we also want to make the UTC versions of the functions. See example: #376

The plan is to make regular functions be based on UTC-functions. First regular functions should convert date arguments from the local timezone to UTC, then pass it to UTC-function and then convert the result back to the local timezone. But how to do so with functions that create the current date internally? My first guess was to make a options.now argument which would be defaulted to new Date() like this:

import endOfToday from 'date-fns/endOfToday'

endOfToday() === endOfToday({now: new Date()})

It would make converting "now" date in non-UTC functions to UTC date beforehand possible. But then I realised that endOfToday({now: new Date()}) is just another (and rather inconvinient) way to write endOfDay(new Date()).

So I think that these functions aren't really needed, they make things difficult and we shouldn't had made them in the first place.

Please tell me what you think about all these functions and about this issue.

Upgrade guide (draft):

  • distanceInWordsToNow(date) -> distanceInWords(new Date(), date)
  • isFuture(date) -> isAfter(date, new Date())
  • isPast(date) -> isBefore(date, new Date())
  • endOfToday() -> endOfDay(new Date())
  • endOfTomorrow() -> endOfDay(addDays(new Date(), 1))
  • endOfYesterday() -> endOfDay(subDays(new Date(), 1))
  • startOfToday() -> startOfDay(new Date())
  • startOfTomorrow() -> startOfDay(addDays(new Date(), 1))
  • startOfYesterday() -> startOfDay(subDays(new Date(), 1))
  • isToday(date) -> isSameDay(new Date(), date)
  • isTomorrow(date) -> isSameDay(date, addDays(new Date(), 1))
  • isYesterday(date) -> isSameDay(date, subDays(new Date(), 1))
  • isThisSecond(date) -> isSameSecond(date, new Date())
  • isThisMinute(date) -> isSameMinute(date, new Date())
  • isThisHour(date) -> isSameHour(date, new Date())
  • isThisWeek(date) -> isSameWeek(date, new Date())
  • isThisISOWeek(date) -> isSameISOWeek(date, new Date())
  • isThisMonth(date) -> isSameMonth(date, new Date())
  • isThisQuarter(date) -> isSameQuarter(date, new Date())
  • isThisYear(date) -> isSameYear(date, new Date())
  • isThisISOYear(date) -> isSameISOYear(date, new Date())
@silvenon

This comment has been minimized.

Copy link
Contributor

@silvenon silvenon commented Jan 7, 2017

I totally agree with this, these functions really aren't needed. I'm always in the favor of removing things! 🔥

@leshakoss leshakoss self-assigned this Jan 9, 2017
leshakoss added a commit that referenced this issue Jan 9, 2017
leshakoss added a commit that referenced this issue Jan 9, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Jan 12, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Jan 15, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Jan 16, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
@leshakoss leshakoss modified the milestone: v2.0.0 Jan 19, 2017
leshakoss added a commit that referenced this issue Jan 20, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Jan 21, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Feb 13, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Feb 28, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Mar 28, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue Apr 14, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
leshakoss added a commit that referenced this issue May 20, 2017
…377) (#380)

* Remove all functions that create the current date internally

* Correct number of functions in README.md

* Regenerate index.js

* Add #377 entry to CHANGELOG.md

* Add reasoning for removing now fns to changelog
@gajus

This comment has been minimized.

Copy link

@gajus gajus commented Jun 25, 2017

I don't know if I would have gone as far as removing them. A better solution would have been to separate the core lib and introduce a common helper collection.

It is a lot harder to read the intent of isSameDay(dateObject, new Date()) than of isToday(dateObject). This is going to encourage developers to implement these abstractions locally with their own quirks to avoid the boilerplate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.