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

Implement simple fromNow() API based on timeago.js #83

Merged
merged 22 commits into from
May 21, 2018

Conversation

aalises
Copy link
Contributor

@aalises aalises commented May 1, 2018

Implementation of the fromNow() function of moment based on timeago.js . The API is as follows:

dayjs().fromNow(dayjs)

And returns a string with the time passed between the two dates with the following format:

Just now , in X hours/years/months , X hours/years/months ago ...

For the sake of simplicity, the time is adjusted to the highest time measurement instead of being unit defined, e.g 60 days will be 2 months , 94670856 seconds will be 2 years, and 76 seconds will be 1 minute , and so on.

Tests are also implemented and passing, which compare the function output with timeago() in various scenarios.

TO DO: Maybe adding some template to change the locale of the messages (change X years ago for X years, or just X...) or to be able to customize them as moment, but to be fair I don't see it as necessary, just adding the chinese version should be it, but it could certainly be implemented. Adding a few lines of code, the version with a specific unit specified (I want it defined in weeks), can also be put, but that would be maybe complicating it a lot?

@codecov-io
Copy link

codecov-io commented May 1, 2018

Codecov Report

Merging #83 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #83   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     12    +1     
  Lines         275    288   +13     
  Branches       47     50    +3     
=====================================
+ Hits          275    288   +13
Impacted Files Coverage Δ
src/locale/es.js 100% <ø> (ø) ⬆️
src/plugin/fromNow.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00acd63...76ae72e. Read the comment docs.

… instead of years, etc), changed some names for constants.
@aalises aalises mentioned this pull request May 1, 2018
@iamkun
Copy link
Owner

iamkun commented May 1, 2018

Perfect thanks. see my comment #50

src/index.js Outdated
const resabs = Math.abs(result)
let out = ''
for (let i = 0; i < P.length; i += 1) {
if (resabs >= P[i].v) out = `${Utils.absFloor(resabs / P[i].v)} ${P[i].l}${(resabs / P[i].v !== 1) ? 's' : ''}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment to this part:
(resabs / P[i].v !== 1) ? 's' : ''
Probably this is equal to:
resabs !== P[i].v ? 's': ''
without division it must be a bit faster :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much @Imperat ! it is indeed cleaner and faster. Just pushed the change (some merge conflicts have to be resolved though 👍 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, @aalises!

@aalises
Copy link
Contributor Author

aalises commented May 9, 2018

@iamkun implementation of the timeago with the locales. TO IMPROVE: Maybe in the spanish local find a way to use other properties of the exported object as a template for the plural forms (to reduce code), maybe using getters...

Also fixed the add method with the locales (was returning a dayjs object without a specified locale), do not know if this is the correct way to do it (maybe cloning it and then setting the date would be better)

@iamkun
Copy link
Owner

iamkun commented May 9, 2018

@aalises Thanks your lovely work. It's great.

Some questions:

  • why compare this with timeago.js instead of moment.fromNow in test case?
  • don't understand here

    spanish local maybe using getters

  • Thanks for the bug fix.

    Also fixed the add method with the locales

Something I'm thinking:

I think we might better add fromNow as a plugin instead of adding to the core code. We may have to add a lot more functions to meets others need such as timezone, UTC support, calendar time in the future. And if we add every thing to our core code, we can't control the total size. (Another moment in size may be). That's why we have a plugin system now.

And we will treat these plugins (mentioned above ) as Day.js extensions with official support rather than user's customized plugin. And added these to our main repo and released with core and locale to NPM.

And if you agree with me, another problem comes. Should we add locales used by plugins to our main locale file? If so, the bundle size will increase. If not, how could we ship these plugins' locale?

P.S. join us at #123 about locale && plugin

@aalises
Copy link
Contributor Author

aalises commented May 10, 2018

@iamkun of course! making a plugin out of it would be great. Timeago.js was used for the test cases for its simplicity, but we can use moment as well! and the getter thing was to simplify the locale by reusing object properties to be part (as a string) of another one, but nvm.

@iamkun
Copy link
Owner

iamkun commented May 10, 2018

I tend to use moment.js. And any simple example code of 'getter'? Still can't get it, sad.

@iamkun iamkun mentioned this pull request May 10, 2018
@aalises
Copy link
Contributor Author

aalises commented May 14, 2018

@iamkun implemented fromNow as a plugin! Still have to figure out what to do with the locales as the plugin sort of extend them (adding, the future, past and now fields and the plural forms), should we put this fields separately inside the plugin or just add them to the locales?

@iamkun
Copy link
Owner

iamkun commented May 14, 2018

Nice plugin!
I think, the en (english) locale should comes with plugin, while others should be in separate locales respectively. How do you think?
see and
But would you mind just wait a moment. There's a little problem with our locale system and the bundle packager. I will fix it in #141. And there might be some code or folder structure change.

@aalises
Copy link
Contributor Author

aalises commented May 14, 2018

Perfect! let me know when it's fixed 👍 So you suggest putting them on plugin/locale?

@iamkun
Copy link
Owner

iamkun commented May 14, 2018

Sure. I mean just put them on locale/es but add en locale to the plugin itself, cause its our default locale. You could check the link in my reply above. But still, I'm not sure if it is the best solution.

@aalises
Copy link
Contributor Author

aalises commented May 15, 2018

Adapted it to the release plugin and locale. However, for the next and current locales to work, the plural forms also need to be specified (see the spanish local as an example). WDYT? @iamkun

@iamkun
Copy link
Owner

iamkun commented May 15, 2018

@aalises good job thx, two questions here.

  • 1 how about move en (English) locale to plugin rather modify the CONSTANT file.
    reference here

  • 2 still don't know why using timeago.js not moment.js for testing

@@ -155,44 +155,44 @@ describe('Difference', () => {
expect(dayjsA.diff(dayjsC, unit, true)).toBe(momentA.diff(momentC, unit, true))
})
})
})
Copy link
Owner

Choose a reason for hiding this comment

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

why this removal? and why the formatting blew?

@@ -0,0 +1,28 @@
import * as C from '../constant'
import * as _U from '../utils'
Copy link
Owner

Choose a reason for hiding this comment

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

no need import, plugin could use Utils through this.$utils()

reference here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! Also, just for consistency and getting rid of a line in the method

    const Cs = this.$locale().M ? this.$locale() : C

Should we put the C.D , C.M ... inside the en export? and use it in index.js not as C. something but referencing the locale

package.json Outdated
@@ -6,7 +6,7 @@
"types": "index.d.ts",
"scripts": {
"test": "jest",
"lint": "./node_modules/.bin/eslint src/* test/* build/*",
"lint": "./node_modules/.bin/eslint src/* test/* build/* --fix",
Copy link
Owner

Choose a reason for hiding this comment

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

no --fix, this could cause a CI pass(auto fix) but not qualified code.

@aalises
Copy link
Contributor Author

aalises commented May 15, 2018

@iamkun yes! moving it into the plugin would keep the constant file leaner, but any other locale needs to have the plural forms implemented.

And I used timeago.js instead of moment because moment is more complex and wouldn't pass the checks (timeago only has X time ago, in X time, and now), while moment has a lot more semantics. I preferred to keep it simple.

@iamkun
Copy link
Owner

iamkun commented May 15, 2018

All right, just timeago.js at the moment.

Plus, what I mean is to move en locale to plugin only, and leave the reset to their local files.

That is to say, all plugin comes with a built in en locale, the same as core module. But if you want to use other locales, you have to import them ahead.

@iamkun
Copy link
Owner

iamkun commented May 15, 2018

Also, consider moment.js's local file:

relativeTime : {
            future : 'en %s',
            past : 'hace %s',
            s : 'unos segundos',
            ss : '%d segundos',
            m : 'un minuto',
            mm : '%d minutos',
            h : 'una hora',
            hh : '%d horas',
            d : 'un día',
            dd : '%d días',
            M : 'un mes',
            MM : '%d meses',
            y : 'un año',
            yy : '%d años'
        },

locale comes with a namespace relativeTime looks easy to understand.

And is un minuto, 1 minuto the same? Don't know Spanish 😬 .

@aalises
Copy link
Contributor Author

aalises commented May 15, 2018

Yep! @iamkun I have a question regarding how to update the locale. Right now I have:

export default (o, c, d) => {
  const proto = c.prototype
  proto.fromNow = function (input) {
    const U = this.$utils()
    d.en = {
      ...C.en,
      MS: 'millisecond',
      MSS: 'milliseconds',
      S: 'second',
      SS: 'seconds',
      MIN: 'minute',
      MINS: 'minutes',
      H: 'hour',
      HH: 'hours',
      D: 'day',
      DD: 'days',
      W: 'week',
      WW: 'weeks',
      M: 'month',
      MM: 'months',
      Q: 'quarter',
      QQ: 'quarters',
      Y: 'year',
      YY: 'years',
      DATE: 'date',
      past: '%s ago',
      present: 'just now',
      future: 'in %s'
    }
    const loc =  this.$locale();
...

However this.$locale() is not updated. What should I use there? I am not sure about what d means here...

@iamkun
Copy link
Owner

iamkun commented May 15, 2018

plugin(option, Dayjs, dayjs)

so d for dayjs function.

you could move your code above proto... jist like advancedFormat.

And a namespace is needed so that you could just do

d.en.relativeTime = {}

instead of merge the object.

replied on my phone, sorry for the code format.

Plus, sorry for the lack of docs, I'll finish it tomorrow.

@aalises
Copy link
Contributor Author

aalises commented May 15, 2018

@iamkun great! will make those changes sometime tomorrow when I can find some spare time :)

@aalises
Copy link
Contributor Author

aalises commented May 16, 2018

@iamkun I pushed the changes yesterday night. With that, everything should be done by now I think. Now the locales implement relativeTime for the plugin. WDYT?

@iamkun
Copy link
Owner

iamkun commented May 16, 2018

looks great. BTW, what's the reason for changing or formatting test/display.test.js file ?

@aalises
Copy link
Contributor Author

aalises commented May 16, 2018

@iamkun lint autofixer did it for me, you can revert it if you consider it!

@iamkun
Copy link
Owner

iamkun commented May 16, 2018

@aalises I think it's not because of the lint auto-fixer. Seems you've deleted one line of code L#158.

@aalises
Copy link
Contributor Author

aalises commented May 16, 2018

@iamkun Yes, when I first coded the fromNow tests they were on the display.test files under the Difference describe block. When moving them to a sepparate file y accidentally deleted the closing bracket it seems, but did not crash because it was still opened at the bottom. Now the block is correctly scoped

@iamkun
Copy link
Owner

iamkun commented May 16, 2018

@aalises Got it. Looks good to me. Will merge into next release. THX 😀

@aalises
Copy link
Contributor Author

aalises commented May 16, 2018

@iamkun perfect thanks! we should put on the documentation some template for the locales as now the relativeTime should be part of it in order for the plugin to work.

@iamkun
Copy link
Owner

iamkun commented May 16, 2018

@aalises I am doing the docs. tough work 😬

@iamkun
Copy link
Owner

iamkun commented May 19, 2018

Will release in 1.6.4

@iamkun iamkun merged commit 97763f8 into iamkun:master May 21, 2018
@chao
Copy link

chao commented May 24, 2018

Great!

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.

5 participants