jasonmit / ember-cli-moment-shim

ember-cli shim for momentjs
MIT License
33 stars 59 forks source link

Implement Ember.Copyable #148

Closed Techn1x closed 4 years ago

Techn1x commented 6 years ago

Obviously it's easy to copy a moment just by wrapping it in moment(), but if I wanted to copy a moment using Ember.copy(value, true) then moment needs to implement Ember.Copyable for this to work

Is this possible?

A use case could be having an object like the following;

let momentHash = {
  momentA: moment(),
  momentB: moment(),
  momentC: moment.duration(6, 'hours'),
}

Then wanting to do Ember.copy(momentHash,true)

Thoughts?


Issue originally opened here; https://github.com/stefanpenner/ember-moment/issues/263

jasonmit commented 6 years ago

@Techn1x thanks for opening the issue, it's certainly something I'd also like to see. Will try and prioritize it if no one gets around to it.

Techn1x commented 6 years ago

@jasonmit Thanks for the response! I believe @mpirio already has a working solution and will be making a PR soon https://github.com/mpirio/ember-cli-moment-shim/tree/feature/implementEmberCopyable

mpirio commented 6 years ago

Hello,

When you import moment from 'moment', moment is not an Ember.Object anymore since 3.0.0 (cf #97). So we can not implement easily Ember.Copyable (with Ember.Object.extend, etc).

When we use Ember.copy(momentObj), the method use Ember.Copyable.detect(momentObj) to know if momentObj implements Ember.Copyable before use its copy method (cf copy.js#L93).

If we read Ember.Mixin.detect() code, we can see that a Meta object is retrieved from momentObj and the Mixin GUID (here it's Copyable GUID) is searched in.

So we need to add a Meta object in momentObj, register Ember.Copyable in this Meta object and define a copy function in momentObj.

I try this here in my fork and it seems to works.

What do you think about?

jasonmit commented 6 years ago

Truthfully, I'm torn about reintroducing the EmberObject wrapper around moment given the complexity it adds. Each method that returns a moment object would need to be reimplemented and the return value wrapped in our EmberObject "wrapper".

If it's avoidable, I'd prefer to go in that direction but this line may be an issue: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/mixin.js#L601

That said, if it's unavoidable then we can go back to an EmberObject.

mpirio commented 6 years ago

I understand. That's why I do not propose to export an Ember.Object. I just did a PR (#149) but the principle is identical (reimplementation of each moment method, wrapped return values, etc).