timrwood / moment-es6

MIT License
3 stars 0 forks source link

Feedback about attaching methods to the prototypes #3

Closed timrwood closed 9 years ago

timrwood commented 9 years ago

I don't like the way you attach stuff to the moment and duration prototypes. Wouldn't it be better if the separate files just define one or more related functions, and then the central file just attaches all functions with a factory or directly. That way you have a quick glimpse at the entire API, in one file. Also I think the dependency graph would be much simpler. Individual function files would be simpler and won't depend on anything except utilities.

Via @ichernev

trek commented 9 years ago

I'm also not a fan of the progressive prototype extension but if you import everything and build up a more complete prototype, nobody can tree-shake away parts they don't use :(

timrwood commented 9 years ago

Yeah, I'm not sure how much tree shaking would be possible as moment is mostly just one big Moment class.

It would be weird to say that you need to import "moment/units/months"; in order to allow moment().format('MMMM').

ichernev commented 9 years ago

If you really want to shake stuff out, it means you're 1) hardcore and 2) willing to fix the es6 build errors saying (can not import file) by commenting out the corresponding Foo.prototype.bar = bar (where bar is imported from deleted file). I think this is fair.

timrwood commented 9 years ago

@ichernev, yeah, we can have everything that is currently attaching itself to the prototype export the method instead, and have one file to collect everything and assign it.

// lib/moment/diff.js
export function diff (input, units, asFloat) {
  ...
}

// lib/moment/prototype.js
import { Moment } from "./constructor";
import { diff } from "./diff";

Moment.prototype.diff = diff;
ichernev commented 9 years ago

Yes, that was my suggestion :)

trek commented 9 years ago

@timrwood that'd be good. And people could build up their own Moment manually if they really wanted a smaller profile?

timrwood commented 9 years ago

This is done for the moment prototype in eaa06b71a6db7ebe2c691af71500582c8522aafd.

timrwood commented 9 years ago

This is done for the locale prototype in ccb406e.

timrwood commented 9 years ago

This is done for the duration prototype as well.