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

Binding problem with 1.0.2 #3

Closed thejaff2 closed 7 years ago

thejaff2 commented 7 years ago

Just updated from 0.3.4 to v1.0.2 via npm. Both value.bind (which worked in 0.3.4)

<abp-datetime-picker value.bind="startdate | dateFormat"></abp-datetime-picker>

and model.bind

<abp-datetime-picker model.bind="startdate"></abp-datetime-picker>

yields this error (and browser hangs with 30% cpu):

Deprecation warning: value provided is not in a recognized ISO format. moment construction falls back to js Date(), which is not reliable across all browsers and versions. Non ISO date formats are discouraged and will be removed in an upcoming major release. Please refer to http://momentjs.com/guides/#/warnings/js-date/ for more info. Arguments: [0] _isAMomentObject: true, _isUTC: false, _useUTC: false, _l: undefined, _i: Invalid date, _f: undefined, _strict: undefined, _locale: [object Object] Error at Function.createFromInputFallback (http://localhost:9000/scripts/vendor-bundle.js:30109:668) at fb (http://localhost:9000/scripts/vendor-bundle.js:30232:129) at qb (http://localhost:9000/scripts/vendor-bundle.js:30298:413) at pb (http://localhost:9000/scripts/vendor-bundle.js:30298:274) at ob (http://localhost:9000/scripts/vendor-bundle.js:30296:274) at rb (http://localhost:9000/scripts/vendor-bundle.js:30303:160) at sb (http://localhost:9000/scripts/vendor-bundle.js:30303:194) at a (http://localhost:9000/scripts/vendor-bundle.js:30092:239) at AbpDatetimePickerCustomElement.valueChanged (http://localhost:9000/scripts/vendor-bundle.js:52618:43) at BehaviorPropertyObserver.selfSubscriber (http://localhost:9000/scripts/vendor-bundle.js:22755:48) at BehaviorPropertyObserver.call (http://localhost:9000/scripts/vendor-bundle.js:22621:14) at BehaviorPropertyObserver.setValue (http://localhost:9000/scripts/vendor-bundle.js:22601:18) at Binding.updateTarget (http://localhost:9000/scripts/vendor-bundle.js:11255:27) at Binding.call (http://localhost:9000/scripts/vendor-bundle.js:11270:16) at SetterObserver.callSubscribers (http://localhost:9000/scripts/vendor-bundle.js:6765:19) at SetterObserver.call (http://localhost:9000/scripts/vendor-bundle.js:10087:12)

ghiscoding commented 7 years ago

Yes I saw this problem after releasing it, that happens when not using an ISO format on the date. After seeing this, I was thinking that it might not have been the best to have 2 bindable attributes driving the picker. Perhaps we should only have 1 bindable attribute (the model.bind) and leave the value.bind as a simple formatted output but not an input.

Here's a simple explanation with the code

this.model = moment(e.date).toDate();
this.value = moment(e.date).format(format);

The piece of code causing the issue is moment(yourFormattedDate), if it's not ISO, moment will throw a warning about the non-ISO format. For example this format YYYY-MM-DD hh:mm is ISO and is ok but if you just add (AM/PM) to it like this YYYY-MM-DD hh:mm a then it will throw the warning. Since the value.bind is the formatted date, it will try to pass it through moment (every single time) and fail if not ISO.

Any thoughts on what else we could do? I'm not sure if a valueConverter would really help in this case... don't think so

ghiscoding commented 7 years ago

Thinking more about this and I actually think that using valueConverter on your side would probably be the best thing to do. If you really want to use a format that is not an ISO format then use a value converter on the value.bind and make sure that the value itself remains a valid ISO format. Unfortunately that would also mean to pass 2 different format, it goes back to your first comment in previous issue.

Something along the lines of

<abp-datetime-picker value.bind="startdate | dateFormat: 'YYYY-MM-DD h:mm a'" format="YYYY-MM-DD h:mm"></abp-datetime-picker>

value converter (code is actually coming from your first comment in previous issue)

import * as moment from 'moment';
export class DateFormatValueConverter {
   toView(value, format: string = "YYYY-MM-DD"): string { 
    return moment(value).format(format)
  }

  fromView(str: string, format: string = "YYYY-MM-DD"): Date {
    return moment(str, format, true).isValid()
      ? moment(str, format).toDate()
      : null;
  }
}

The value converter would only be used if you want a value.bind that is not a valid ISO format. In most of my cases, I usually use a valid ISO format (like 'YYYY-MM-DD') so I'm typically good without a value converter. I could add a note in the readme about this ISO warning. I could also modify my code to throw an error if the provided value.bind format is an invalid ISO format, in that case it would tell the user to add a value converter avoid having the CPU go 30% like you said (which I also did see, a day after I had already pushed the version 1.x).

If you have better suggestions, please let me know.

thejaff2 commented 7 years ago

But when using only model.bind using a date object ("startdate") you shouldn't get this error I suppose?

<abp-datetime-picker model.bind="startdate"></abp-datetime-picker>

ghiscoding commented 7 years ago

The reason is because they affect each other. Changing value.bind or model.bind will have the same result and they are both calling moment. If your start date is not a valid ISO format in both, it will throw the same warning. What is your start date format?

thejaff2 commented 7 years ago

In my ViewModel I have (I am not storing any string representation of the date at all)

let startdate: Date = new Date(2017, 1, 1);

and in my view I have

<abp-datetime-picker model.bind="startdate"></abp-datetime-picker>

ghiscoding commented 7 years ago

Hmm ok, I'm not sure if we can pass a Date object directly to moment. Sorry, I'm not in front of my code but what happen if you do this:

let startdate: Date = new Date(2017, 1, 1);
console.log(moment(startdate));

Does it work or does it throw the ISO format warning?

thejaff2 commented 7 years ago

Nope, it works fine:

let s: Date = new Date(2017, 1, 1); console.log(moment(s));

image

ghiscoding commented 7 years ago

Trying the sample I made for client-wp and changing the model.bind to your date this.myDateObject = new Date(2017, 1, 1); and it works fine on my side.

Can you troubleshoot with Chrome and tell me where/how it fails?

You could also try with my sample

git clone https://github.com/ghiscoding/Aurelia-Bootstrap-Plugins
cd client-wp
npm install
npm start

In the constructor, I have this.myDateObject = new Date(2017, 1, 1); and the output is shown below (month start at 0, so st of February shows up in print screen). Also in the sample, I have 2 buttons that can drive both value.bind (left button) and model.bind (right button). They both do what they're suppose to do.

The only thing I might want to add in the plugin is a check on model.bind to make sure it's a Date object while also checking that value.bind is a valid ISO date (if not throw a msg to the user asking them to plug a valueConverter)... but that won't help with your issue.

datepicker

ghiscoding commented 7 years ago

Any update on your issue? If not I'll eventually close the issue. Thanks

ghiscoding commented 7 years ago

Took some time to review this in the weekend and found the issue which was throwing a "moment ISO incorrect date format" message when the model.bind is/was empty at the project creation. I had incorrect code to use moment() with a null date in the bind function, which was causing the issue. Please update NPM to latest version 1.0.3 of aurelia-bootstrap-datetimepicker.

You might also be interested in my latest plugin. Aurelia-Bootstrap-Select on NPM / source Bootstrap-Select

Thanks for your help and patience. :) Also please upvote if you like the plugin ⭐️

thejaff2 commented 7 years ago

Thank you. Got stuck with other projects so I didnt have time to respond, sorry.

On Mon, Apr 10, 2017 at 5:13 AM, Ghislain B. notifications@github.com wrote:

Took some time to review this in the weekend and found the issue which was throwing a "moment ISO incorrect date format" message when the model.bind is/was empty at the project creation. I had incorrect code to use moment() with a null date which was causing the issue. Please update NPM to latest version 1.0.3 of aurelia-bootstrap-datetimepicker https://www.npmjs.com/package/aurelia-bootstrap-datetimepicker.

You might also be interested in my latest plugin. Aurelia-Bootstrap-Select https://github.com/ghiscoding/Aurelia-Bootstrap-Plugins/tree/master/aurelia-bootstrap-select on NPM https://www.npmjs.com/package/aurelia-bootstrap-select / source Bootstrap-Select http://silviomoreto.github.io/bootstrap-select/

Thanks for your help and patience. :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ghiscoding/Aurelia-Bootstrap-Plugins/issues/3#issuecomment-292839874, or mute the thread https://github.com/notifications/unsubscribe-auth/AY8y3cuaFpAbkyuyvj2BQuPcYhjHh1jhks5ruZ5QgaJpZM4MtANv .

ghiscoding commented 7 years ago

no problem, please give it a try and let me know if that fixes it ;)