Provide an ember-data "moment" transform

[Panman82]

It could be used in places where DS.attr('data') is currently used. Ex: DS.attr('moment')

[jasonmit]

I would be in favor of this being its own addon versus implementing ember-data specific logic in the addon.

[Panman82]

Why? It's just providing a transform, not forcing users to use it. And it's moment.js specific...

[jasonmit]

It's ember-data specific, not everyone uses ember-data so not everyone should pay the cost. It might be less of an issue if the builds shake unused modules, but that's not the case today.

[Panman82]

What about just leaving it in the addon folder? Then users could add the transform into their app when needed and import the actual transform from this addon. Provides an "opt-in" feature with no cost.

P.S. I also can't wait for tree-shaking. 😄

[jasonmit]

The addon folder still gets bundled with the app, but we could create a blueprint for this so users that want this transform can opt into it with ember g ember-moment-transform and it will drop the transform where it's needed, app/transforms/moment.

Do you want to spike this PR?

[Panman82]

Humm... another good idea. But I thought the addon folder was not included unless the addon also had an "importing" file in the app folder.?.

Sure, I'll try to look into it this weekend (family time permitting).

[jasonmit]

The addon folder is used for namespacing, to prevent collision with the app namespace.

[Panman82]

So I started to do some digging and found out that there already is an addon for moment transforms. https://github.com/pk4media/ember-cli-moment-transform I'm torn between continuing with blueprints here or pointing folks to the other addon... Thoughts?

[jasonmit]

@Panman8201 some issues there, that addon uses the obsolete bower dep of ember-cli-moment-shim instead of the ember-addon. That would need to be solved first.

https://github.com/pk4media/ember-cli-moment-transform/blob/master/blueprints/ember-cli-moment-transform/index.js#L12

ember-moment uses the npm dep: https://github.com/stefanpenner/ember-moment/blob/master/blueprints/ember-moment/index.js#L11

[Panman82]

Yeah, I noticed that. There is an open PR to fix it though, but no action from the repo owner. (sad)

[jasonmit]

Lets just go with the blueprint approach, it makes the most sense and it doesn't look like that project is actively managed anyway.

[jasonmit]

Any progress here @Panman8201? I'll likely pick it up soon if you haven't started.

[stefanpenner]

Im actually -1 on this, not because it isn't a cool feature. But because when this is done, it is almost always a hefty performance issue in apps. Moment is just not terribly quick, and people tend to load lots of ember-data models. Often times resulting in transforms that aren't even used, rather making these transforms HTMLBars helpers is usually the right balance to address the perf isuse.

[jasonmit]

I didn't consider the performance impact. But I can imagine a scenario where perf is actually worse if each time you need to act on the date attr you need to convert the date into a moment object, I can think this throwing off the perf trade off in the other direction.

I guess the real question is weather transforms happen lazily when accessing attrs or up front (I don't know, but I'd guess during serialization/deserialization).