cibernox / ember-power-calendar

Powerful and customizable calendar component for Ember
http://www.ember-power-calendar.com
Other
211 stars 118 forks source link

Potentially remove ember-moment dependency #127

Closed mazondo closed 7 months ago

mazondo commented 6 years ago

In doing a quick audit of our dependencies, I see that ember-power-calendar has ember-moment listed as a dependency. This adds about 21k to the minified vendor build. When I went through the source, it looks like it's mostly a test dependency, with one exception:

https://github.com/cibernox/ember-power-calendar/blob/49eff727309fbe77ca117062382698feb5c41fe6/addon/templates/components/power-calendar/nav.hbs#L9

Would you be open to a PR removing ember-moment as a dependency and moving it to a devDependency for tests only?

boyanyordanov commented 6 years ago

Pretty sure it also uses the shims that come with ember-moment to be able to import moment as a module. Just used that to set a timezone globally so that power-calendar respects it like the rest of the app.

cibernox commented 6 years ago

It does use moment in many places because using dates in JS without some sort of tool (it being moment.js, date.js or luxor) is pretty inconvenient.

To be clear, I would totally accept a PR removing moment.js, but it would take some work and it's not a personal priority for me because I'm using moment.js anyway in the app.

If you want to tackle it tho, I'd be glad to assist.

mazondo commented 6 years ago

Yeah I wouldn't try to repace moment, just ember-moment. ember-cli-moment-shim lets you import moment, but ember-moment adds a lot of weight with convenience methods.

rtablada commented 6 years ago

I think removing ember-moment-shims would be 👍 for me. The new import support for Ember CLI would mean that users may start using non-shimmed moment soon (we're looking at it in some of our apps)

mazondo commented 6 years ago

I'm actually not as worried about ember-moment-shims, but once ember-cli updates are in places of course we could and should. I'm referring to ember-moment, which is a convenience library that adds some nice to have handlebars sugar, but at the cost of a lot of weight.

I haven't had time to look at this yet, but ember-moment adds 21k in handlebar helpers, that's in addition to moment itself.

rtablada commented 6 years ago

@cibernox I think a step toward this would be to use a blueprint to install ember-moment in the host app.

Right now I was really excited to think we had removed reliance on ember-moment-shim only to find that it was still included as a dependency of ember-power-calendar.

With ember-auto-import and the soon to land packager, I think it's more up to host apps to provide import moment from 'moment' either through shims or true imports.

rtablada commented 6 years ago

So actionable steps could be:

  1. Move ember-moment to blueprint install (and remove ember-moment and ember-moment-shims from dependencies)
  2. Start removing references to helpers provided by ember-moment (still relying on moment IMO is ok)
  3. Add logic to either install moment-shim or moment depending on CLI import ability
cibernox commented 6 years ago

I totally agree, moment should be optional. I plan to tackle this myself soon (weeks) as one app that is using it right now will migrate from moment.js to luxon.

rtablada commented 6 years ago

I'm not sure how simple abstracting moment/luxon will be since they have different API surfaces (and the abstraction would likely be fairly large in size).

My goal isn't to limit moment use (although that could help). But instead reduce or remove the need for ember-moment and the helpers/code that it adds to applications. And to remove the dependency on ember-cli-moment-shims since now with packager and ember-auto-import, host applications can import moment from 'moment' just by npm install moment (reducing the load and addon tree for the shim package). Removing ember-cli-moment-shim also allows better typesense since language servers are able to more easily read type definitions when an app has moment as a app level dependency.

cibernox commented 6 years ago

Well, the idea is not use ANY. All the operations on dates will be done with just the native Date object (they are not so many). The value received by actions will have { date: <the-date> } instead of { date: <the-date>, moment: <moment-version-of-the-same-date> }.

If you happen to have moment.js the addon will detect it and add the moment key to the object. And if you have luxon it will do the same with a datetime key (or luxon, tbd)