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

Added a plugin system. #92

Merged
merged 5 commits into from
May 7, 2018
Merged

Added a plugin system. #92

merged 5 commits into from
May 7, 2018

Conversation

schiem
Copy link
Contributor

@schiem schiem commented May 2, 2018

The plugin system allows users to extend dayjs with additional plugins,
or change existing functionality.

The plugins work by calling the extend method:

dayjs.extend({
  n: "methodName",
  m: function(arguments) {
     //
  }
})

When overwriting a function, the original function can be referenced from
inside the plugin using:

this.getOld(pluginName)

For example:

dayjs.extend({
  n: "format",
  m: function(formatStr) {
    let oldFormat = this.getOld("format")
    let oldResults = oldFormat(formatStr)
    ...
})

This commit also introduces a plugin which extends the format options to include:
-Q: Quarter of the year
-k/kk: 1-24 hour time
-ddd: Date of the month with ordinal (1st-31st)
-S: Milliseconds

The plugin system allows users to extend dayjs with additional plugins,
or change existing functionality.

The plugins work by calling the extend method:

dayjs.extend({
  n: "methodName",
  m: function(arguments) {
     //
  }
})

When overwriting a function, the original function can be referenced from
inside the plugin using:

this.getOld(pluginName)

For example:

dayjs.extend({
  n: "format",
  m: function(formatStr) {
    let oldFormat = this.getOld("format")
    let oldResults = oldFormat(formatStr)
    ...
})

This commit also introduces a plugin which extends the format options to include:
-Q: Quarter of the year
-k/kk: 1-24 hour time
-ddd: Date of the month with ordinal (1st-31st)
-S: Milliseconds
@schiem schiem mentioned this pull request May 2, 2018
@codecov-io
Copy link

codecov-io commented May 2, 2018

Codecov Report

Merging #92 into @next will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           @next    #92   +/-   ##
====================================
  Coverage    100%   100%           
====================================
  Files          3      3           
  Lines        206    183   -23     
  Branches      39     28   -11     
====================================
- Hits         206    183   -23
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/utils.js 100% <0%> (ø) ⬆️
src/constant.js 100% <0%> (ø) ⬆️

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 ab67853...4f9eb23. Read the comment docs.

@iamkun iamkun added the discussion Further discussion label May 3, 2018
@schiem
Copy link
Contributor Author

schiem commented May 3, 2018

@Frondor had suggested a system more like Vue js, where the prototype is passed into the plugin function and the plugin is able to alter it however it would like. It would look something like:

src/index.js

Dayjs.prototype.extend = (fn) => {
  fn(Dayjs)
}

User's app.js

dayjs.extend( (Dayjs) => {
    let oldFormat = Dayjs.prototype.format
    Dayjs.prototype.format = (formatStr) => {
      let oldResponse = oldFormat(formatStr)
      ...
     }
})

Both that approach and the current one should work equally well, it seems to be a matter of preference and how much control plugin authors should be given over the default DayJS object.

@iamkun
Copy link
Owner

iamkun commented May 3, 2018

@schiem I agree giving plugin a full access to the Dayjs class. That makes it a real plugin 😋

@iamkun
Copy link
Owner

iamkun commented May 3, 2018

Plus, I thinks we might better use Utils rather than this.$u in our main code. This will reduce the code size after minimize.

@Frondor
Copy link
Contributor

Frondor commented May 3, 2018

Just one thing, you may want to define whether you want to extend prototype or __proto__ (class vs instance).

In Vue you can't do new Vue({...}).use(myPlugin). Your are supposed to install the plugin first, and then instantiate the Vue object which shall use all registered plugins, like Vue.use(myPlugin); const app = new Vue({...}). That's why I wanted to export Dayjs class in first place.

I know its a total different approach, but just take a look, and tell later: https://codepen.io/Frondor/pen/MGopZx?editors=0010
Influenced by the current Vue source code. It adds a new feature called hooks. While the plugin is executed as soon as it is registered / installed, the user can use plugin hooks to set information at instantiation time.

// The plugin
const lastNamer = {
  install (Person, opts = {}) {
    const formatter = s => {
      return typeof opts.formatter === 'function' ? opts.formatter(s) : s
    }

    Person.addHook(function (p) {
      p.lastName = formatter(p.cfg.lastName)
    })

    Person.prototype.getName = function () {
      console.log('I am', `${this.firstName} ${this.lastName}`)
    }
  }
}

// Plugin installation with options
Person.extend(lastNamer, {
  formatter: ln => ln.toUpperCase()
})

// Library usage
const person = new Person({
  firstName: 'Jon', // supported by default
  lastName: 'Snow' // supported after plugin
})

console.log(person)
person.getName() // new custom method

@harrysarson
Copy link

harrysarson commented May 3, 2018

If I can weigh in with my two cents:

From the README:

💪 Immutable

I believe it would be great to implement plugins in a way which extends this concept of immutability.

By this I mean, instead of mutating dayjs.prototype (which is effectively a global variable) when extending dayjs with plugin, they would instead write:

 const extended_dayjs = dayjs.extend(plugin);

This would leave the original dayjs unchanged and the user can use their new, shiny extended_dayjs however they wish.


Edit: My opinion here also applies to any localisation dayjs lets users set which is in conflict with existing work in #90.

@Frondor
Copy link
Contributor

Frondor commented May 3, 2018

Have to agree with harry. Also, take in mind that after #90, you will be able to do

import { Dayjs } from 'dayjs'

class MyDayjs extends Dayjs {
  constructor (cfg) {
    super(cfg)
  }
}

Edit: My opinion here also applies to any localisation dayjs lets users set which is in conflict with existing work in #90.

@harrysarson the localization approach is fully immutable besides of the setLocale method. You're always supposed to create your base instance (with the locales you need) and then use it normally with clone() / set(), etc...

myDayjs.format('dddd') // "Monday"
myDayjs
    .add(1, 'day')
    .setLocale(eng) // set the same locales just for this example sake
    .format('dddd') // "Tuesday"
myDayjs.format('dddd') // "Monday"

@schiem
Copy link
Contributor Author

schiem commented May 3, 2018

@Frondor The person example you gave is very close to what's being done here, but the extend method is placed on the dayjs "factory" that has access to the Dayjs class, and that's used to extend the prototype. The main difference is that the install is handled internally, so the user just needs to pass in the plugin name and method to be run. Personally, I like removing as much of the boiler plate for the plugin author as possible, but I think that's a matter of preference.

I also really like the idea of hooks, I'll give that a look.

@harrysarson I agree that it would be good to maintain immutability, but I can only think of two ways to do it with a plugin system:

  • Plugins only apply to an individual instance
  • Adding a plugin creates a new subclass with the extended method, and the extend method returns a new "factory" for that subclass.

Which leaves us with:

  • Decide that we want to ignore immutability and override the global Dayjs prototype
  • Implement one of the above options (or another one that I haven't thought of)
  • Decide we don't need a plugin system

As a side note, I dislike the idea of exposing the Dayjs class to the end user. It seems to me that destroys a lot of the immutability--with the current set up, the end user can't (reasonably) access the underlying Dayjs class, so the library is completely immutable. Exposing the underlying class opens it up for any user to alter the internals.

@harrysarson
Copy link

Again just my personal views here:

@Frondor

besides of the setLocale method

I think a setLocal method that returned a new instance of dayjs rather than modifying the current instance would be better.

@schiem

  • Adding a plugin creates a new subclass with the extended method, and the extend method returns a new "factory" for that subclass.

I think this would be a sensible approach.

@iamkun iamkun mentioned this pull request May 3, 2018
@iamkun
Copy link
Owner

iamkun commented May 3, 2018

I agree that immutability matters. But consider a real project example like this:

// assumed we are running a Spanish site
// in root.js or index.js init dayjs settings
import dayjs from 'dayjs'
import { Locale_ES } from 'dayjs/locales' // Spanish locale constant
import { Plugin_TimeAgo } from 'dayjs/plugins' // extend dayjs#fromNow 
dayjs.extend([Locale_ES, Plugin_TimeAgo])// set locale && plugin

// for the rest of the code in this project
dayjs().fromNow() // -> new API 1 year ago... 
dayjs().set(...).format() // format in Spanish

Although, it changes the original Dayjs, it's easier to read and understand (at my point).
And the only one more API our user has to learn is .extend() (or .use() maybe)

We might cloud have a second argument for .extend(obj or array, isReturnNewDayjs = false) to our advanced user or user who cares more about immutability. In this way

// same as above
const extendedDayjs = dayjs.extend([Locale_ES, Plugin_TimeAgo], true) // no change to original dayjs but return a new one
dayjs().format() // default English
extendedDayjs().format() // Spanish

Plus, in my approach, I've treated locales as a kind of extension, so did plugins. So that we might could minimize the new added APIs and concepts.

@schiem
Copy link
Contributor Author

schiem commented May 3, 2018

@iamkun That makes sense to me. Any thoughts on whether it makes more sense to have the Dayjs core handle installing the plugins vs. having the plugins install themselves using an install method?

Having the core install them would reduce the complexity for the plugin author, but it would also reduce the amount of flexibility the plugin author has.

@iamkun
Copy link
Owner

iamkun commented May 4, 2018

@schiem I've made a simplified version base on @Frondor's code above.

//-----core.js-------
import LocaleDefault from 'LocaleDefault' // english
class Dayjs {
  getLocale() {
    return LocaleDefault                  // override this to return new locale
  }

  format(string, anotherLocale) {
    return anotherLocale ? anotherLocale.weekName || this.getLocale().weekName      // use this.getLocale() to get correct locale
  }
}
const dayjs = config => new Dayjs(config)
dayjs.extend = (cb, isNew = false) => {   // isNew -> immutable or not
  if (isNew) {
    class newDayjs extends Dayjs {
    }
    cb(newDayjs)
    return config => new newDayjs(config)
  } else {
    cb(Dayjs)                   
  }
}
export default dayjs
//-----plugin.js-----
const PluginWeek = (vm) => {
  vm.prototype.week = function (proto, dayjsClass) {
    return this.$W
  }
}
const LocaleSpanish = (vm) => {
  const LocalSpanishObject = { weekName: 'spanish week' }
  vm.prototype.getLocale = function () {
    return LocalSpanishObject
  }
}
//-----usuage.js----
// set global
dayjs.extend(PluginWeek)
dayjs.extend(LocaleSpanish)
dayjs().week() // new API
dayjs().format() // format with spanish locale

const extendedDayjs = dayjs.extend(PluginWeek, true) // get new class instance for immutability

schiem added 2 commits May 4, 2018 23:42
…ugin authors.

Plugins now take the Dayjs (or extended subclass) as a parameter and are responsible
for installing themselves.

The extend function is chainable, for example:
dayjs.extend(plugin1).extend(plugin2)

The extend function also now takes a second boolean parameter, which will return a new
version of dayjs. Example:

var extended = dayjs.extend(plugin1, true)
extended().plugin1()
dayjs().plugin1() //Error
@schiem
Copy link
Contributor Author

schiem commented May 5, 2018

I've incorporated those changes to the plugin system. I'll leave the locale system to @Frondor.

You can still use the plugin system via

dayjs.extend(plugin)

But you can now pass in true to return a new factory. It's now also chainable, so you can do:

dayjs.extend(plugin1).extend(plugin2)

Or

var extended = dayjs.extend(plugin1, true).extend(plugin2)

extended().pluginFunction1()
dayjs().pluginFunction1() //Error

Plugins are now passed in the class object that they're modifying, so they can modify them however they need to, like so:

export default myPlugin = (Dayjs) => {
    Dayjs.prototype.myPlugin = function() {
        ...
    }
}

@iamkun
Copy link
Owner

iamkun commented May 5, 2018

@schiem Looks good.

  1. Check out my comment above.

Plus, I thinks we might better use Utils rather than this.$u....

  1. Another problem here, should we make a backup for our core APIs, may be like this.$old you used before. If on plugin modified the behavior of one core API, other plugin may also affected, if they relay on the same API. Here coms the question you've mentioned yesterday,.

Any thoughts on whether it makes more sense to have the Dayjs core handle installing ...

@schiem
Copy link
Contributor Author

schiem commented May 6, 2018

I've switched the this.$u to use the Utils library directly in the core, and I've swapped this.$u with a function that returns the Utils library for plugins to use, like so:

this.Utils().padStart(...)

The reason for that change is that this.$u had the Utils library assigned to it for every dayjs instance that was created, whereas this.Utils() should always return a reference to the same library.

For point 2, I think that since we're passing in the Dayjs prototype for plugins to modify, it makes sense for them to handle the install themselves (since there might be plugins, like locales, that might modify a variable instead of a method). And if they're handling the install themselves, then I think it might make sense for them to keep track of the original function themselves. This would also reduce conflicts when installing multiple plugins that operate on the same method.

For instance, say there were two different format plugins that added slightly different functionality. If the core was responsible for keeping track of the original format method, then you might end up with something like:

dayjs.extend(format1) //dayjs().format() now calls original format, then format1
dayjs.extend(format2) //dayjs().format now calls original format, then format 2, but never format 1

If the plugins are responsible for saving the original method, then they should be storing whichever method is currently on the plugin. For example:

plugin1 = dayjs => {
    const oldFormat = dayjs.prototype.format //oldFormat is the original format
    dayjs.prototype.format = (formatStr) => {
        ...
    }
}

dayjs.extend(plugin1) //dayjs().format() is now calling plugin1 format, which calls the original format

plugin2 = dayjs => {
    const oldFormat = dayjs.prototype.format //oldFormat is now plugin1 format
    dayjs.prototype.format = (formatStr) => {
        ...
    }
}

dayjs.extend(plugin2) //dayjs().format now calls plugin2 format, which calls plugin 1 format, which calls the original

It also gives the plugin authors the opportunity to not call the original if they need to completely change the core functionality. I think that not enforcing a specific install method gives the plugin authors a lot more freedom to extend in the way they need to.

@iamkun
Copy link
Owner

iamkun commented May 7, 2018

I'm gonna to merge this pr fist. And put locale && plugin 's code altogether. Then we might could discuss the new release in one pr.

@iamkun iamkun merged commit b36bdcf into iamkun:@next May 7, 2018
@Frondor Frondor mentioned this pull request May 9, 2018
@xx45
Copy link
Contributor

xx45 commented May 15, 2018

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants