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

String 'null' displayed as date value when using model.bind #17

Closed don-bluelinegrid closed 7 years ago

don-bluelinegrid commented 7 years ago

See attachment -

Using a model.bind like this:

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

results in the component showing 'null' when a Date object is not provided in the binding. This is not expected design, as there should not be a requirement for a value/object in the binding initially - it should be possible to display the component with a value not yet set, for example, to support a use case where setting a date is optional, not required.

Forcing a value/object to be required eliminates the possibility of using this component in such a use case.

Thanks, Don

screen shot 2017-06-15 at 9 42 21 am
don-bluelinegrid commented 7 years ago

Through experimentation, I found that I could fix this by providing a value.bind experession with a blank space character:

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

However, this seems problematic because:

  1. Using an empty string '' did not work, which seems pretty inconsistent.
  2. This is undocumented, and I doubt that anyone would understand that this is how the component works.

It seems that this is forcing an empty formatted value as the initial value - and therefore bypassing the display of 'null' for the empty object value. The component should just do a check for a null object in model.bind, and display an empty string in that case.

Don

don-bluelinegrid commented 7 years ago

Ignore last comment. This is not a solution.

While that approach does initialize the input element as empty (instead of 'null'), it causes an error to be thrown when the datepicker is opened:

Uncaught Error: Binding expression "' '" cannot be assigned to.
    at LiteralString.assign (aurelia-binding.js:1132)
    at Binding.updateSource (aurelia-binding.js:4819)
    at Binding.call (aurelia-binding.js:4841)
    at BehaviorPropertyObserver.callSubscribers (aurelia-binding.js:325)
    at BehaviorPropertyObserver.call (aurelia-templating.js:3552)
    at TaskQueue.flushMicroTaskQueue (aurelia-task-queue.js:147)
    at MutationObserver.<anonymous> (aurelia-task-queue.js:75)

How can this be resolved?

ghiscoding commented 7 years ago

is there a particular reason you use null? I've set my examples to just declare the variable and nothing shows up in the input which is what you want. I understand that null should be permitted, and I will look into fixing this, but I'm just giving you another option.

don-bluelinegrid commented 7 years ago

I'm not sure what you mean "use null". I am not "using null".

I have a model object, called Task. A Task has a 'dueDate' property. This property is optional, and will be set by the user, at the time of creation. In the Task model object, the 'dueDate' property is declared, but is not initialized. So, it is initially undefined. This property is never set to null.

So, I don't see anywhere in my code where I would be setting any bound value to null. Furthermore, I believe that the way your component works is that you are converting the model.bind object into a formatted string, using the the format options property. So, it's likely that this is where the null value is introduced - when trying to produce a formatted string from an empty object value. It's possible there my be some exception being thrown, that isn't being caught or logged.

If you can explain to me what you mean by "I've set my examples to just declare the variable and nothing shows up in the input", i.e., which variable you're referring to (remember, I don't plan to use a value.bind), then I'm willing to try that.

Thanks, Don

ghiscoding commented 7 years ago

Perhaps I got fooled by the output being null, not necessarily your input. You didn't mention undefined until now, so I got the wrong assumption.

When I say my examples, I really meant the Aurelia-Bootstrap-Plugins - examples that I provided in the project, for example client-wp (WebPack). With the previous issue, I've added an example with only a value.bind and another with only a model.bind to cover all possibilities.

Can you provide some code for the View and ViewModel so I can add them to my examples and fix the issue when I have time later tonight or tomorrow. Or if you want to look into the plugin source code and provide a PR, I would be super happy as well.

Thanks

don-bluelinegrid commented 7 years ago

I think I provided all of the information you would need to reproduce this:

Model:

I have a model object, called Task. A Task has a 'dueDate' property (type JS Date). This property is optional, and will be set by the user, at the time of creation. In the Task model object, the 'dueDate' property is declared, but is not initialized. So, it is initially undefined. This property is never set to null.

View:

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

Unfortunately, with the demands of my product releases, I wouldn't have time to work on this type of PR.

Thanks, Don

ghiscoding commented 7 years ago

I tried multiple ways and I can't replicate your issue. I always get nothing showing in the input (empty input). I tried with undefined, null and also defining the variable without initializing it. In every cases, I get nothing showing inside the input until I choose a date.

This is what I tried with TypeScript.

export interface Task {
  dueDate: Date;
}

export class MyClass {
  task: Task = { dueDate: undefined };
  // task: Task = { dueDate: null };
  // task: Task;
  // ...
}
 Task: ${task | stringify}
<div class="form-group">
     <label class="control-label">Date Entered 5</label>     
     <abp-datetime-picker class="form-datepicker" element.bind="picker" model.bind="task.dueDate" options.bind="{ showTodayButton: true, format: 'YYYY-MM-DD' }"></abp-datetime-picker>
</div>

What shows up for undefined and null is an empty object { }, while if task: Task (not initialized), then the output is nothing inside `. In all tests, I never havenull` showing in the input.

Below is the output I get (nothing showing in the input) when I use task: Task = { dueDate: undefined }; or task: Task = { dueDate: null };

image

don-bluelinegrid commented 7 years ago

Hi Ghislain -

Regarding your questions:

There is clearly something different happening between your test code and mine, because I use the datetimepicker component in several places, and in each place I get the same behavior - null appears in the input element on initialization. If you have a look at the attached screen shot, you can see a plain input field, and the datetimepicker below; the plain input doesn't display anything, the datetimepicker displays null. Also, I only noticed that this behavior began after I upgraded from 0.3.4 to 1.0.9.

screen shot 2017-06-18 at 10 55 55 am

I've looked at your example above, and I'm not seeing the obvious difference between your code and mine, so I agree this is a tricky one to figure out.

Regarding your out-of-topic question - I would give a star to a project after I've been using it in a commercial/production project for a certain period of time, with no problems. In other words, after I've had time to validate that it is both useful and robust - I believe that's the meaning of the star. I communicate fairly regularly with Rob and several other members of the Aurelia team through GitHub, regarding various parts of the product that they're working on - and I've found bugs/problems in some of those projects as well.

I'm very familiar with open source and what it is, and having been using and contributing to OS projects for about 17 years. I think a component like datetimepicker is something that's needed for Aurelia. In my opinion, I don't think this component is quite ready for inclusion, and one reason is the design that we've touched on - there seems to be a slight contradiction of the intention of how bound model objects should be used in a component, and the use of a native Date object as a representation of a date. If you'd be interested in a code review at some point, when my project permits, I'd be willing to do that. A couple of initial comments I had when quickly reviwing the code are:

In abp-datetime-picker.js in the bind() method, why isn't the code using the standard Aurelia originalBindingContext and parentBindingConext parameters?

In the same bind() method, why isn't the code relying on the standard Aurelia binding pattern, and why is the code trying to get an object directly from a DOM element instance?

Thanks, Don

ghiscoding commented 7 years ago

Hello, thanks for the feedback. If you ever find out where the issue is coming from, that would be great since I really can't reproduce on my side.

As for the code review, sure I would be more then willing to change some pieces of my code implementation. Why am I not using the Aurelia way of doing things? Simply because I still lack of experience and training, I coded all of that with what I have learned so far. I find that there's a big lack in available training and tutorial for Aurelia, which is kinda sad since it's really a nice framework but compare that to what's available for Angular2+ and wow do we got a lot more training available there. You're dealing with Aurelia for work, while I'm coding with Aurelia so far only in my free time, so I'm not quite at your level of knowledge on this framework, however I appreciate feedback and help on building/testing these plugins which hopefully helps others.

Thanks for the feedback

ghiscoding commented 7 years ago

Found the issue, it was caused by this external PR #16 which added the placeholder functionality but should have been set to empty string when not provided. Please use latest version 1.0.10

don-bluelinegrid commented 7 years ago

Interesting though, that you could not reproduce the issue. Did you have this PR integrated into the version of the plugin that you had been testing?

Also, as a side note - When a PR is submitted, I believe that it should not have been submitting the compiled (grunt/gulp built) code. It should have submitted the modified/changed source code; code reviewer should have reviewed the source code changes (and either approved code or requested changes); then the project owner should build the source code, test it, and merge to the master branch, and build/check-in the distributables. It looks like in this case, you allowed the submitter to do the build/check-in of distrubatables, before the source code was tested locally.

Don

ghiscoding commented 7 years ago

I was able to reproduce the issue today, which is how I found the problem. Not sure why I couldn't see it before as you said. It might be because my package.json wasn't set to latest version.

I merged the PR as I didn't see any issues at that time, since my examples didn't have null or undefined values at that time. I know that you wish this project would be fully tested and everything but I have a full time job too and it doesn't include Aurelia. This is an open source project which I handle in my free time. I wish that I could devote more time, but that is still limited and we don't use Aurelia at work (my manager doesn't want at this point), so hopefully some day I will implement unit tests but until then, I rely on the open community to report/fix issues, that's all I can say.