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

"Cannot read property 'options' of undefined" when date picker is inside a repeater #50

Closed pajohns closed 6 years ago

pajohns commented 6 years ago

I'm trying to use the abp-datetime-picker and it works perfectly except for when I include it inside a table where the rows are repeater objects. My html code is as follows:

enter-payslip.html

<table>
    <tr repeat.for="day of payCycle.days">
        <td><abp-datetime-picker options.bind="pickerOptions"></abp-datetime-picker></td>
    </tr>
</table>

And in the .ts file

pickeroptions = {
    format: 'YYYY-MM-DD'
};

main.ts

aurelia.use
    .standardConfiguration()
    .feature(PLATFORM.moduleName('resources/index'))
    .developmentLogging()
    .plugin(PLATFORM.moduleName('aurelia-bootstrap-datetimepicker'));

The stacktrace is:

aurelia-task-queue.js?2bf4:44 Uncaught TypeError: Cannot read property 'options' of undefined
    at AbpDatetimePickerCustomElement.optionsChanged (abp-datetime-picker.js?01f5:330)
    at BehaviorPropertyObserver.selfSubscriber (aurelia-templating.js?8628:3674)
    at BehaviorPropertyObserver.call (aurelia-templating.js?8628:3539)
    at BehaviorPropertyObserver.setValue (aurelia-templating.js?8628:3519)
    at AbpDatetimePickerCustomElement.descriptor.set [as options] (aurelia-templating.js?8628:3629)
    at AbpDatetimePickerCustomElement.attached (abp-datetime-picker.js?01f5:130)
    at Controller.attached (aurelia-templating.js?8628:3468)
    at View.attached (aurelia-templating.js?8628:1518)
    at ViewSlot.add (aurelia-templating.js?8628:1681)
    at Repeat.addView (repeat.js?f59c:241)
    at ArrayRepeatStrategy._standardProcessInstanceChanged (array-repeat-strategy.js?2d28:105)
    at ArrayRepeatStrategy.instanceChanged (array-repeat-strategy.js?2d28:29)
    at Repeat.itemsChanged (repeat.js?f59c:136)
    at BehaviorPropertyObserver.selfSubscriber (aurelia-templating.js?8628:3674)
    at BehaviorPropertyObserver.call (aurelia-templating.js?8628:3539)
    at BehaviorPropertyObserver.setValue (aurelia-templating.js?8628:3519)
    at Repeat.descriptor.set [as items] (aurelia-templating.js?8628:3629)
    at Object.setValue (aurelia-binding.js?5f98:3630)
    at Binding.updateTarget (aurelia-binding.js?5f98:4882)
    at Binding.call (aurelia-binding.js?5f98:4897)
    at SetterObserver.callSubscribers (aurelia-binding.js?5f98:328)
    at SetterObserver.call (aurelia-binding.js?5f98:3703)

I've tried creating a custom template and using it in there, but the same thing happens. If it's just in a table ie not in a row repeater it seems to work fine.

ghiscoding commented 6 years ago

Not sure if that would help in your case, but I pushed a new version yesterday that brings the functionality to be able to dynamically change any options. You can see that in the demo, that is the last one down below

pajohns commented 6 years ago

Hi, yes I've pulled the latest version just earlier today. It doesn't seem to matter which way I choose for the options whether they're in the html components or in the .ts files, it still throws this exception. I haven't tried dynamically assigning the options, but it isn't really what I'm trying to achieve - may be worth a shot though to see if it troubleshoots it.

ghiscoding commented 6 years ago

I would be glad if you could troubleshoot and come up with a fix.

Have you tried to pass the option directly in the bind instead of a variable? <td><abp-datetime-picker options.bind="{ format: 'YYYY-MM-DD' }"></abp-datetime-picker></td>

There seems to be some kind of timing issues somewhere

thejaff2 commented 6 years ago

I'm also having this problem. in a table row (even when not inside a repeater if that is significant). I just checked, and this problem did not exist in version 1.2.0, so it should be easy to track down.

pajohns commented 6 years ago

@ghiscoding I'd love to help but I'm really new to Aurelia - I did look into it a bit and I can see that the options is null when processing this block of code:

this.options = Object.assign({}, globalPickerOptions, pickerOptions);

I'll try and look into it more, but it's really a bit above my level of understanding at the moment. I did try, as you suggested, to do an inline bind, but it's still throwing the same error.

ghiscoding commented 6 years ago

Ok that is a good start, that line is to merge the 2 objects into 1 object. I'm a bit surprised it would throw an error though. Anyhow this line can also be written this way this.options = { ...globalPickerOptions, ...pickerOptions };

Not sure if that makes any differences, it's technically supposed to be doing the same behavior which is really to merge 2 objects into 1 (the object on the right has priority, so if the properties exist in both it will take the object property from pickerOptions in our case). That technique is not restricted to Aurelia, it's really an ES6 thing, which is kind of confusing the first time you see it.

For more info on these subjects, you can read Object.assign and the Spread Operator. I started using TypeScript after creating this lib and they strongly suggest to use the Spread Operator, so I'm using the spread nowadays.

pajohns commented 6 years ago

Ok so I've spent more time wrapping my head around all things Aurelia to be able to include the project locally to muck around - tried your suggestion but no luck. However I got further along to realize where null pointer is. I'm pretty sure it's in the optionsChanged method, specifically on line 324...

this.domElm.data('DateTimePicker').options(newValue);

is trying to find DateTimePicker which at this point isn't returning anything. Any ideas on this?

pajohns commented 6 years ago

Per @thejaff2 it was the latest fix that has caused the issue. Removing the entire method...


  optionsChanged(newValue, oldValue) {
    if (newValue !== oldValue && newValue && this.domElm) {
      let newFormat = newValue.format;
      if (newFormat && this._format !== newFormat && moment(this.model, newFormat).isValid()) {
        this._format = newFormat;
        this.model = moment(this.model, this._format).toDate();
      }
      this.domElm.data('DateTimePicker').options(newValue);
    }
  }

fixed the problem on my end. I'll have a look more into it, but hope this has given some help.

ghiscoding commented 6 years ago

That is a good finding, however this new optionsChanged is a rather nice new addition that I would prefer to keep. This was added to bring ability to dynamically change any options of the picker.

If it's simply a missing if condition somewhere, that would be good to work. is it possible that your for loop changes some of the options that you are not aware of?

pajohns commented 6 years ago

I added an additional null check and this seems to have fixed the problem - pretty sure this wouldn't have broken any existing functionality, but not sure if you add a dropdown selector for a repeater line if it'd work - not sure of the use case for this ever being needed though. I've branched on my end but can't push my branch to you without permission. Here's the code...

optionsChanged(newValue, oldValue) {
  if (newValue !== oldValue && newValue && this.domElm && this.domElm.data('DateTimePicker')) {
    let newFormat = newValue.format;
    if (newFormat && this._format !== newFormat && moment(this.model, newFormat).isValid()) {
      this._format = newFormat;
      this.model = moment(this.model, this._format).toDate();
    }
    this.domElm.data('DateTimePicker').options(newValue);
  }
}

RE "for loop changes some of the options", I don't think so. The options change function is getting called on instantiation when the page is getting built and at the time the options object is fine - it's simply not able to find the DateTimePicker object within domElm.

ghiscoding commented 6 years ago

can you also provide a code sample that can be added to the demo page, that also help in making sure everything works before releasing a new version. Thanks

pajohns commented 6 years ago

Just using the code I provided at the beginning should be enough for testing...

<table>
    <tr repeat.for="line of someDataObjectWithMultipleValues">
        <td><abp-datetime-picker options.bind="pickerOptions"></abp-datetime-picker></td>
    </tr>
</table>

Just any background data should do.

ghiscoding commented 6 years ago

I guess that I also need the content of someDataObjectWithMultipleValues variable

pajohns commented 6 years ago

This is the data that was failing for me...

{
  "days": [
    {
      "id": 1,
      "date": "2018-01-11T00:00:00",
      "timeStart": "2018-01-11T07:30:00",
      "timEnd": "2018-01-11T22:00:00",
      "breaks": [
        {
          "id": 1,
          "breakStart": "2018-01-11T11:30:00",
          "breakEnd": "2018-01-11T12:00:00"
        }
      ],
      "overnightHoursStart": "2018-01-12T02:00:00",
      "overnightHoursEnd": "2018-01-12T05:00:00",
      "isSleepover": false,
      "hoursOvernight": 6,
      "isPublicHoliday": false,
      "tenHourBreakGiven": false,
      "normalHours": 9,
      "saturdayHours": 5,
      "sundayHours": 9,
      "publicHolidayHours": 9,
      "eveningHours": 4,
      "nightHours": 7,
      "overnightPenalty": 1,
      "mealWithClientCount": 1,
      "overtimeHalf": 2,
      "overtimeDouble": 1,
      "totalHours": 5
    },
    {
      "id": 2,
      "date": "2018-01-11T00:00:00",
      "timeStart": "2018-01-24T08:30:00",
      "timEnd": "2018-01-24T22:00:00",
      "breaks": [
        {
          "id": 14,
          "breakStart": "2018-01-24T11:30:00",
          "breakEnd": "2018-01-24T12:00:00"
        }
      ],
      "overnightHoursStart": "2018-01-25T01:00:00",
      "overnightHoursEnd": "2018-01-25T03:00:00",
      "isSleepover": false,
      "hoursOvernight": 1,
      "isPublicHoliday": false,
      "tenHourBreakGiven": false,
      "normalHours": 3,
      "saturdayHours": 2,
      "sundayHours": 8,
      "publicHolidayHours": 7,
      "eveningHours": 6,
      "nightHours": 8,
      "overnightPenalty": 1,
      "mealWithClientCount": 0,
      "overtimeHalf": 8,
      "overtimeDouble": 7,
      "totalHours": 8
    }
  ]
}
ghiscoding commented 6 years ago

Thanks I will add that in the demo but that might take a couple days before I get time to do this. I'm unfortunately not using Aurelia at work and so all the Aurelia stuff is done during my spare time.

Thanks for all the feedback and help troubleshooting the issue.

pajohns commented 6 years ago

No probs - no better way to learn how something works than when you fix it :)

ghiscoding commented 6 years ago

Thanks for all the feedback and help, I included the fix that you provided and pushed a new version 1.3.1 on NPM

Please confirm that it works

pajohns commented 6 years ago

Perfect! I notice that you are also now enforcing that the object is a date object. This caused me some problems because for some reason when pulling in from my API the date fields are being set as strings rather than casting as date objects. IE my 'Day' interface originaly had a 'Date' object for each of the start time, finish time etc, but I ended up going back to string given that they would end up as strings anyway. Weird. But yes, working fine now in a repeater list 👍

ghiscoding commented 6 years ago

Thanks for confirming, you can also upvote ⭐️ if you haven't already. Enjoy