feature(datepicker): let users define his own date model implementation

[edriang]

Please remember, the issues forum is NOT for support requests. It is for bugs and feature requests only.
Please read https://github.com/ng-bootstrap/ng-bootstrap/blob/master/CONTRIBUTING.md and search
existing issues (both open and closed) prior to opening any new issue and ensure you follow the instructions therein.

Bug description:

The user is forced to use NgbDateStruct as his application-model for dates.
If as a user I want to manage the dates using native JavaScript Date class, then I first need to convert Date to NgbDateStruct before assigning it to the date-pickers' model.
Then, before persisting the model I need to convert back from NgbDateStruct to Date.
It could be nice that NgbDateParserFormatter let the user to define methods to convert from user-model date implementation to ng-bootstrap/datepicker NgbDate implementation, and then back from NgbDate to user-model date implementation.

Link to minimally-working plunker that reproduces the issue:

You can fork a plunker from one of our demos and use it as a starting point.
Please note that we can not act on bug reports without a minimal reproduction scenario in plunker. Here is why:
https://github.com/ng-bootstrap/ng-bootstrap#you-think-youve-found-a-bug

Version of Angular, ng-bootstrap, and Bootstrap:

Angular: 4.0.3

ng-bootstrap: 1.0.0-beta.1

Bootstrap: 4.0.0-beta

[pkozlowski-opensource]

This is duplicate of the existing issue #754

[edriang]

@pkozlowski-opensource I know about format and parse functions but I don't think this solves my current problem.
One thing is how to show the selected date to the user. This can be handled by the format function OK.
Another thing is how to parse from the user INPUT to the internal date format used by the datepicker. This can be handled by the parse function OK.

But If I want to pass a Date object as the initial ngModel value to the datepicker I can't, because it is expecting NgbDateScruct as input. (Maybe a fromInput method?)
On the same way, when a date is selected ngModel is always bound to a NgbDateScruct, but what if I want to use a Date object for example?? (Maybe a toOutput method?)

I want to submit a PR that issues these problems, exposing two methods fromInput and toOutput, that by default behave the same as now (receiving and returning a NgbDateScruct object) but can be customized for the user.

Do you understand the problem? If there is another way to solve this please let me know.

[pkozlowski-opensource]

@edriang I do understand the issue, yes. And if you read through, more specifically here: #754 (comment) you will see:

How do use strings as a model
There is not great story for this as there is a missing robust support for model transformation in the angular itself. See discussion in the timepicker-related issue for more details: #545

This is why this issue is open as we need to be able to handle the case you are describing.

I want to submit a PR that issues these problems, exposing two methods fromInput and toOutput, that by default behave the same as now (receiving and returning a NgbDateScruct object) but can be customized for the user.

Awesome! We are busy with shipping 1.0 but this part of the datepicker is high on our priority list so any help would be more than appreciated!

Now, could you, before starting any work on a PR, prepare a short design and put it in #754? This would allow us to validate our understanding / thinking and avoid back-and-forth on the actual PR.

cc @maxokorokov

[edriang]

Thanks @pkozlowski-opensource, I needed this feature right now so I've already worked on a patch in a fork.

Let me know if you can see this commit details: edriang@683116a

I will try to explain the modifications below:

• I've added two extra methods to NgbDateParserFormatter. One used to create internal NgbDate from external model (aka fromInput), and another to convert back from NgbDate to external model (aka toOutput).
• I've made these changes in NgbDateParserFormatter because that DI token was used to perform custom date-transformations.
• A default implementation of fromInput and toOutput is provided in NgbDateISOParserFormatter in order to continue current support as always, expecting and returning NgbDateStruct
• I needed to create a new NgbDateService class in order to Inject NgbDateParserFormatter and create NgbDate object
• I used NgbDateService.create() method in datepickerComponent.writeValue method to init from external model
• I used NgbDateService.getValue() method in datepickerComponent.onChange method to output internal NgbDate to external model on each selection

In this way, I think that internal implementation of the datepicker should behave as always. At least all test cases were passed successfully.

Let me know if you need some extra detail or if you think on a better approach to solve this.

[pkozlowski-opensource]

@edriang 2 quick thoughts (more tomorrow):

• I think that this should be a separate service, not new methods on NgbDateParserFormatter - the simple reason for it is that you might want to have certain string format in input but another shape for models. Those things don't need to change together and shouldn't be part of the same class.
• we need more tests for corner cases - one that I want to see for sure is that we are not re-creating internal objects (not creating new objects) when the model value doesn't change.

[pkozlowski-opensource]

Re-opening as we start to have some good discussion here!

[edriang]

@pkozlowski-opensource Great!, I will break those methods into a separated class.

Any suggestion for the class name? Maybe something like NgbDateModelParser?

I open also to suggestions about the methods names. Currently fromInput and toOutput.

Thanks

[pkozlowski-opensource]

Any suggestion for the class name? Maybe something like NgbDateModelParser?

NgbDatepickerModelAdapter?

I open also to suggestions about the methods names. Currently fromInput and toOutput.

Hmmm... Not sure. 'modelToStruct' / structToModel?

Also note that material is taking a slightly different approach here: https://github.com/angular/material2/blob/master/src/lib/core/datetime/date-adapter.ts

@maxokorokov WDYT?

[edriang]

Ok @pkozlowski-opensource , I took some of your suggestions with little modifications on names:

• I've split the code into a new class named NgbDateModelAdapter
• Default implementation provided is under NgbDateModelStructAdapter class and configured / provided in DatepickerModule
• Methods names are called modelToNgbDate and ngbDateToModel instead modelToStruct and structToModel (NgbStruct would be the model not the target of conversion`).
• I added some basic tests based on the ngb-date-formatter-parser.spec.ts file.

More details can be seen on the following commit: edriang@29f3a78

Also note that material is taking a slightly different approach here: https://github.com/angular/material2/blob/master/src/lib/core/datetime/date-adapter.ts

I can see they have only one adapter to configure all. In NgBootstrap configuration is split in different classes / services (ie: formatter/parser and I18N)

In NgBootstrap you have NgbDate to manage internal dates, and then NgbStruct to represent selected date model. That's why I think that the solution adding a simple adapter does the trick here without many modifications.

[kelvinlouis]

Is there a way for me to get my hands on this? I really need the modelToNgbDate and ngbDateToModel functionality.

[edriang]

@pkozlowski-opensource any suggestion about how to proceed? Do you need something else before sending the PR? Thanks in advance

[pkozlowski-opensource]

@edriang the proposal sounds good to me but I would love to have input from @maxokorokov (original author and maintainer of the datepicker). You can go ahead with a PR if you are not shying away from back-and-forth on the review :-)

[maxokorokov]

Hi there,

that looks good to me as start, that's definitely one of the top priority things to fix.

Would like to have a couple of questions clarified before you go with the PR:

• why not justNgbDateAdapter? also ngbDateToModel looks redundant to me.

class NgbDateAdapter<T> {
  toModel(NgbDate): T;
  fromModel(T): NgbDate;
}

So implementations would be named NgbDateStructAdapter, NgbDateNativeAdapter, NgbDateMomentAdapter, etc.

• we should use it not only for the ngModel conversion, but everywhere else in the NgbDatepicker like markDisabled, maxDate, minDate and also in DateTemplateContext and NgbDateParserFormatter, right ?

• should we type NgbDatepicker as NgbDatepicker<T> accordingly? should we use any as you do now? I would prefer the first one, but not sure what impact it would have

• does it make sense to have the NgbDateStructAdapter as the default one, or switch to NgbDateNativeAdatper with the default JS date as a model right away?

[edriang]

Hi @maxokorokov, thank you for the suggestions.

I've committed a new version, you can see the changes here: edriang@38b335a

About your questions:

why not justNgbDateAdapter? also ngbDateToModel looks redundant to me.

Done. Liked the renaming suggestions.

we should use it not only for the ngModel conversion, but everywhere else in the NgbDatepicker like markDisabled, maxDate, minDate and also in DateTemplateContext and NgbDateParserFormatter, right ?

I don't know if this is necessary, as a user I only care about ngModel. I mean, I don't have any problem configuring maxDate, minDate, etc, using NgbDateStruct format. I wanted to make few changes as possible regarding internal model manipulation. Do you think this is necessary?

should we type NgbDatepicker as NgbDatepicker accordingly? should we use any as you do now? I would prefer the first one, but not sure what impact it would have

Done

does it make sense to have the NgbDateStructAdapter as the default one, or switch to NgbDateNativeAdatper with the default JS date as a model right away?

I think this is necessary if we want to keep retro-compatibility with actual implementation. Changing default Date model will be a breaking change for anyone using this datepicker

---

With this commit, I've also pushed a modification on the demo project. I've updated the API doc section regarding NgbDateAdapter and also provided a minimal example defining a custom NgbDateNativeAdapter and using it in the example component.

I'll be waiting for any other change or suggestion. Thanks

[sergiofm]

Can't wait to see this avaliable in a release.

[maxokorokov]

@edriang, thanks, great job!

I like it so far, looked through your code, haven't found any major issues for me. Do you mind opening a pull request for this one? I think lots of people would we happy with this change

I don't know if this is necessary, as a user I only care about ngModel. I mean, I don't have any problem configuring maxDate, minDate, etc, using NgbDateStruct format. I wanted to make few changes as possible regarding internal model manipulation. Do you think this is necessary?

It would be nice to have it in the long run. If you're using, say, momentjs in your app, you would expect it everywhere, all datepicker APIs included. But agree that for now ngModel is enough, l'll open a separate issue for this.

I think this is necessary if we want to keep retro-compatibility with actual implementation. Changing default Date model will be a breaking change for anyone using this datepicker

Agreed. Will open another issue to add other default adapters to be bundled with ng-bootstrap.

[edriang]

Hi @maxokorokov, sorry for bothering you. I've tried to pull the request, but I'm having trouble with some ci build task.

I'm getting an error when the build:demo command runs (I'm copying just the first chunk of the error below):

10% building modules 6/8 modules 2 active ...ules/bootstrap/dist/css/bootstrap.cssModuleNotFoundError: Module not found: Error: Can't resolve './../../$$_gendir/demo/src/app/app.module.ngfactory' in '/Users/agallardo/Documents/Proyectos/angular-libs/ng-bootstrap/demo/src'

Do you have any clue what could it be the problem? Because when running the demo with demo-server command, everything goes well.

Thanks in advance for any help.

[maxokorokov]

Hey, @edriang. You have a missing export that fixed the issue for me → see the PR. Webpack error message though...

Also did a quick pass through the code

[saxicek]

Hi @edriang, what is the current state of your implementation? Do you plan the PR or is there any issue stopping you from it? Thank you!

[edriang]

Hi @saxicek, I sent the pull request and made all corrections suggested.

The current status of the PR is OK: #1816
Please let me know if I had to make some additional step in order to merge this PR.

[edriang]

Anyone can merge this? Is there anything or any additional step I'm missing in order to progress?
@pkozlowski-opensource? @maxokorokov ?

[saxicek]

Hi @edriang, it seems like additional work was requested by @maxokorokov. Do you plan to follow up or shall I try to finish it? Thank you!

[adrian-marcelo-gallardo]

Hi @saxicek, sorry, I forgot to answer. I no longer work for the company that used Angular and where I needed this functionality. Feel free to fork the repo and continue. Any help you need you can ask although.