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

Inject moment as a dependency, avoid mutation of moment instance #1

Open
timoxley opened this issue Nov 26, 2014 · 4 comments
Open

Comments

@timoxley
Copy link

Current implementation breaks node's module system by mutating the global instance of moment. i.e. if two packages depend on different versions of moment-precise-range, which version is used? It will depend on the order they're loaded, which could change at runtime.

Secondly, users should have precise control over which version of moment they want/need to use.

Ideally, user passes moment into this plugin like so:

var moment = require('moment')
moment = require('moment-precise-range')(moment)

This removes need for moment-precise-range to even list moment as a dependency (I'd set it as a devDependency though so it's available during testing).

The plugin should not mutate the original moment instance. This means not using the official, yet destructive moment.fn method. Rather, the plugin would do something like this:

module.exports = function(moment) {
   moment = Object.create(moment) // protect original moment from modification
   moment.preciseDiff = preciseDiff
   return moment
}

Alternatively you could export the preciseDiff function in isolation so users can attach it to moment however they wish.

@timoxley
Copy link
Author

Another alternative which doesn't attach anything to moment, just takes a moment instances and passes back a function bound to that instance:

module.exports = function(moment) {
  return function preciseDiff(d1, d2, opts) {
    // etc
  }
}

@timoxley
Copy link
Author

Oh, my bad the Object.create suggestion won't work with moment because it's default export is a function. You can set the prototype directly on the function though, something like this:

module.exports = function(moment) {
  // protect original moment from modification
  var wrappedMoment = function() {
    return moment.apply(this, arguments)
  }
  wrappedMoment.__proto__ = moment
  wrappedMoment.preciseDiff = preciseDiff
  return wrappedMoment
}

@mtscout6
Copy link
Owner

I like where you are going with this. I don't have the time to address it right now, however I do take pull requests.

@psulek
Copy link
Collaborator

psulek commented Mar 24, 2016

@mtscout6 just created pull #5

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

No branches or pull requests

3 participants