danielmoncada / date-time-picker

Angular Date Time Picker (Responsive Design)
https://daniel-projects.firebaseapp.com/owlng/date-time-picker
MIT License
140 stars 60 forks source link

Missing dependency "Moment" on new and upgraded projects, while using OwlNativeDateTimeModule #125

Closed florisvanderhaar closed 2 years ago

florisvanderhaar commented 3 years ago

Even whilst not using Moment.js, there still appears to be a dependency for this date-time-picker. When you try to npm start a new or upgraded project, you will be hit with:

Error: The target entry-point "@danielmoncada/angular-datetime-picker" has missing dependencies:
 - moment

I was able to reproduce the error by creating a new angular app (with Angular Material) and setting up the imports:

@NgModule({
     imports: [ 
         OwlDateTimeModule, 
         OwlNativeDateTimeModule,
     ],

In projects where we used this date-time-picker, we have always used OwlNativeDateTimeModule and never the OwlMomentDateTimeModule, as stated in the documentation. I got the error after upgrading to Cypress 7, so it appears Cypress 6 was our last package that installed moment as a dependency.

As you might know already know, Moment.js is a legacy project. Reason enough to move away from it in new and upgraded projects, as apparently more projects are doing.

GFlores98 commented 3 years ago

In my case I'm using Angular and

Angular CLI: 11.0.7 Node: 14.15.1 OS: win32 x64

And I solved installing moment.

npm i moment

florisvanderhaar commented 3 years ago

Yes, installing Moment.js is a valid workaround for the immediate short term.

However, in doing so I will add a package to my application that's not in active development anymore, while I purposely configured the date-time-picker to use the OwlNativeDateTimeModule instead of Moment. This is a bad practice.

IMHO: The aim should always be to install the least amount of dependencies on a new project and avoid leaving any unwanted, unused or deprecated packages in a project during an upgrade.

Moment.js hadn't been deprecated yet when the date-time-picker was made. But still the documentation wrongly states:

There are two pre-made modules:

OwlNativeDateTimeModule -- support for native JavaScript Date object OwlMomentDateTimeModule -- support for MomentJS object. (need to npm install ng-pick-datetime-moment and moment packages)

Source: https://danielykpan.github.io/date-time-picker/#locale-formats

TLDR; I shouldn't have to install a package I'm not using.

EricDitp commented 3 years ago

Same here, uselessly importing moment, which I don't really want. I would like to remove moment from my project a.s.a.p.

thanks for all the good work.

decline commented 3 years ago

Having the same problem here. Even though I'm using OwlNativeDateTimeModule (and not OwlMomentDateTimeModule), I'm still forced to install Moment.js which will then also end up in my production-bundle, causing an increase of ~350kb. That's just bad 😕

jcompagner commented 3 years ago

i personally find it very weird that it still ends up in the production bundle should angular treeshaking and so on take care of that? Why would angular think it still references somehow?

tmburnell commented 2 years ago

I bet this is the reason we get this error: https://github.com/danielmoncada/date-time-picker/blob/714f752d36184c158481f878a4327b16641e090e/projects/picker/src/public_api.ts#L19

I will do some more testing.

tmburnell commented 2 years ago

I did confirm if you drop

export {
  MomentDateTimeAdapter,
  OwlMomentDateTimeAdapterOptions,
  OWL_MOMENT_DATE_TIME_ADAPTER_OPTIONS
} from './lib/date-time/adapter/moment-adapter/moment-date-time-adapter.class';

and

export { OwlMomentDateTimeModule } from './lib/date-time/adapter/moment-adapter/moment-date-time.module';

from the public_api it no longer errors saying moment is required.

seowzhenjun0126 commented 2 years ago

Hi, is there any update on this? Thanks!

danielmoncada commented 2 years ago

@seowzhenjun0126 no updates, I can look more into it this week.

In order to make the changes @tmburnell suggested, the "Moment Adapter Options" would need to be removed completely from this project, and ported over to a sub-project for those that are using them.

danielmoncada commented 2 years ago

Okay.. finally had time to remove the MomentAdapter, and extract it out into a separate project.

This is in the latest version (https://github.com/danielmoncada/date-time-picker/pull/143) v13.1.0.

If anyone does need a MomentJs adapter, you can find it here npm or GitHub.

seowzhenjun0126 commented 2 years ago

Thank you so much for your work! Appreciate it

danielmoncada commented 2 years ago

@seowzhenjun0126 thanks!