Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binding problem with 1.0.2 #3

Closed
thejaff2 opened this issue Mar 29, 2017 · 12 comments
Closed

Binding problem with 1.0.2 #3

thejaff2 opened this issue Mar 29, 2017 · 12 comments

Comments

@thejaff2
Copy link

thejaff2 commented Mar 29, 2017

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
Copy link
Owner

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
Copy link
Owner

ghiscoding commented Mar 29, 2017

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

thejaff2 commented Mar 30, 2017

Nope, it works fine:

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

image

@ghiscoding
Copy link
Owner

ghiscoding commented Mar 30, 2017

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
Copy link
Owner

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

@ghiscoding
Copy link
Owner

ghiscoding commented Apr 10, 2017

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
Copy link
Author

thejaff2 commented Apr 10, 2017 via email

@ghiscoding
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants