ghiscoding / Aurelia-Bootstrap-Plugins

Aurelia-Bootstrap-Plugins are Custom Elements to bridge with a set of 3rd party Bootstrap addons
https://ghiscoding.github.io/Aurelia-Bootstrap-Plugins
MIT License
45 stars 23 forks source link

model.bind causes exception thrown #13

Closed don-bluelinegrid closed 7 years ago

don-bluelinegrid commented 7 years ago

@ghiscoding

Hi,

I've just now updated to 1.0.7 after the various changes, and I'm seeing a problem.

The expectation with the two-way bound model.bind would be that the control should read the current value and display it appropriately, and should also communicate changes in the control back to the model value.

I'm seeing a problem with the first case, reading the value. I have a model object with a dueDate property, of type Date. When I include the datepicker element and bind it to this property, it is throwing an error:

Uncaught Error: Datetimepicker, model.bind must be of type Date

When I debug through the ABP code, I see that the error is being thrown here:

  AbpDatetimePickerCustomElement.prototype.modelChanged = function modelChanged(newValue, oldValue) {
    if (typeof newValue.getMonth !== 'function') {
      throw new Error('Datetimepicker, model.bind must be of type Date');
    }
    if (newValue !== oldValue && newValue) {
      if ((0, _moment2.default)(newValue, this._format, true).isValid()) {
        this.value = (0, _moment2.default)(newValue, this._format, true).format(this._format);
      }
    }
  };

What seems to be happening is that this function modelChanged is being called three times - the first two times it's called with Date objects for newValue and oldValue, but the third time it's called with newValue as a string "Wed May 31 2017 15:15:22 GMT-0400 (EDT)", which appears to be the Date.toString() of the original Date object. So, this fails the test above. See the attached screen shot. screen shot 2017-06-05 at 11 13 46 am

My code is not doing anything special here. I'm simply including the abp-datetimepicker element and binding it to my model's Date object property.

            <abp-datetime-picker
                element.bind="picker" model.bind="item.dueDate" 
                show-today-button.bind="false" side-by-side.bind="true" format="YYYY-MM-DD"></abp-datetime-picker>

Is there someplace in the ABP code that would be calling modelChanged() after either formatting the model.bind property or calling toString() on it? This is making the component unusable for me currently.

Thanks, Don

ghiscoding commented 7 years ago

Have you looked at the samples I provided (WebPack, CLI or even now asp.net? In the samples, I do value change and a model change without any issues.

There is however something that won't work in your code, since version 1.x all the options have to be called by options.bind and so your format and show-today-button.bind="false" won't work as is. Here's an example on how to use it options.bind="{ showTodayButton: true, format: 'YYYY-MM-DD' }"

Also, the modelChanged could be called multiple times, I didn't know it was as many as 3 times but I can figure out at least 2 (one would be called by the bind() function and then another by the model.bind and perhaps value.bind if provided as well). It's important to know that in the plugin code, one call the other (meaning modelChanged calls valueChanged and vice versa, the if checks inside each are there to block any infinite loop calls).

Can you please update your code to use options.bind and see where that goes.

don-bluelinegrid commented 7 years ago

@ghiscoding

Ghislain,

I changed the options binding as you suggested. As I suspected, this made no difference - I am still getting the same binding error.

You mentioned that it was not unexpected to see the modelChanged() function called 3 times upon initialization - is that really expected? And, why would it be called multiple times, and why would a modelChanged() handler be called as a side-effect of a format() call?

Here is what I see: 1 - newValue: Date object, oldValue: undefined 2 - newValue: Date object, oldValue: Date object 3 - newValue: String (toString of Date), oldValue: Date object => Exception thrown; String is not Date for model.bind

Can you provide any explanation of how my implementation could be affecting this? Or is it possible that your existing users are all using the value.bind, and so may not have run into this?

My current code looks like this:

Template:

            <abp-datetime-picker
                element.bind="picker" model.bind="item.dueDate" options.bind="{ showTodayButton: true, format: 'YYYY-MM-DD' }"></abp-datetime-picker>

Model:

@inject(Session, I18N)
export class CaseResource {

  title: string;
  description: string;
  externalReference: string;
  attachmentCount: Number;
  attachments: Array<any>;
  caseId: string;
  type: any;
  typeId: string;
  status: any;
  priority: string;
  assigneeReference: any;
  dueDate: Date;
  lastExportDate: Date;
  tasks: Array<any>;

Note that the model.bind refers to the dueDate property, which is a Date object.

Thanks, Don Peterkofsky

ghiscoding commented 7 years ago

I might have not explained myself properly, I was expecting at least 2 calls to the modelChanged(). The following 2: 1- called because of the bind() 2- called because you provided a value as dueDate

Not sure why there's a 3rd one (I haven't tried your code), but it might still be normal. I do still say that yes it's not unexpected to have multiple calls (because if you change the model.bind or the value.bind, it will call a modelChanged() or a valueChanged()).

We have to find where the 3rd one comes from and perhaps refine the if() that is inside.

... looking at your debug screen, can you put a break point at line 301 and see if the problem comes from there.

don-bluelinegrid commented 7 years ago

@ghiscoding

Ghislain,

Thanks for getting back on this.

  1. I can't think of why there would be a need to call toString() on the Date object - the only string that should be produced should be from a format() call, and that should not trigger any model change, because it is formatted for the display, not for the model.

  2. In your previous reply, you mentioned about calling modelChanged and valueChanged each time either binding changes. This doesn't seem correct. It seems to me that the model and value are separate concerns and should not be coupled like this. The value is intended to be used only for the formatted display in the component. The model is intended to be used for communicating the data from the component to the ViewModel. They are not tightly coupled. As an example - I used model.bind' but do not usevalue.bind. So, I would think it would be unnecessary and undesirable to be calling avalue.bind` change handler in the case where no binding for this is used - are you checking for that case?

Don

don-bluelinegrid commented 7 years ago

@ghiscoding

To answer your other question -

I do see line 301 being called, but after that it calls modelChanged line 288 two times, and the error is thrown after the second time. So, I think the answer is - no, the line 301 is not the direct cause of the error.

Don

ghiscoding commented 7 years ago

As I wrote before, I think bind() would call a modelChanged at first, could you verify that? I can't see from your debug screen, but just search for the bind function. I just want to make sure it's not that one causing the issue.

don-bluelinegrid commented 7 years ago

I have put a breakpoint in the bind() function. bind() is called long before modelChanged(), and modelChanged() is not called from bind(). In bind() this.model is showing as undefined, as is value.

bind() gets called when the custom component is loaded into the template; modelChanged() gets called when we click on the calendar to open the datepicker. I'm not sure why you were thinking these are called at the same time.

It seems fairly clear that there is a bug here. You should be able to reproduce this very simply, by using the element as you've designed it, with a model.bind value and with no value.bind value. I'm assuming you would already have tested it in this manner - can you continue the debugging this way?

Thanks, Don

ghiscoding commented 7 years ago

Alright, so using in my new ASP.Net Core sample which has TypeScript. I copied the code you mentioned earlier from your View template

<abp-datetime-picker element.bind="picker" model.bind="item.dueDate" options.bind="{ showTodayButton: true, format: 'YYYY-MM-DD' }"></abp-datetime-picker>

then added dueDate: Date; to the ViewModel and I don't have any errors in the console. The element is empty at start and the picker works correctly after choosing a date. So I can't reproduce your issue.

However, looking at your Model, there's no item, even if you are specifically calling model.bind="item.dueDate", is that a typo or something that you omitted?

As a side note, this plugin was completely developed in my spare time. Since this time is limited, please don't assume too many things. I did not test everything and did not had the time to add unit test yet, so thank you for your comprehension.

ghiscoding commented 7 years ago

This should be fixed now with the suggestion provided in #14. Please update to latest NPM version 1.0.8

If it's still an issue, please provide more details or open another issue. Thanks