Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

datepicker: watchers count #2613

Closed
porjo opened this issue Aug 21, 2014 · 20 comments
Closed

datepicker: watchers count #2613

porjo opened this issue Aug 21, 2014 · 20 comments

Comments

@porjo
Copy link

porjo commented Aug 21, 2014

I've been looking at ways to improve performance in my app. I've read that reducing number of unnecessary watchers is something that can help.

Using a simple watch counter I see that the number of watchers increases by 296 when a datepicker is introduced into the page. See this plunker for a demo. AFAIK this is due to the html template using ng-repeat to create the table of days.

Some questions:

  1. Is this a real issue i.e. is my analysis correct?
  2. If it is a problem, can anything be done?
@ValeryIvanov
Copy link

If think that this is a problem that should be thought about, because I see some 'lag' while typing in the input box in my app where on one view I have two datepickers and a lot of other stuff.

@ruben-podadera
Copy link

+1 Same for us :-(

@ny0m
Copy link

ny0m commented Nov 18, 2014

+1

1 similar comment
@ChrisL1200
Copy link

+1

@nadavsinai
Copy link

+299 watchers for me!

@geilt
Copy link

geilt commented Dec 15, 2014

+7000 watchers. I use a table with date pickers. Bad bad idea apparently...

@NoRKin
Copy link

NoRKin commented Dec 16, 2014

+1, each datepicker adds ~300 watchers.

@geilt
Copy link

geilt commented Dec 16, 2014

I looked through the templates and none of them are using 1.3's BindOnce. I assume they should be considering it is a calendar and the dates shouldn't change too often? Thing is NG-IF is being use so should take care of most of the watchers but it isn't. Had to stop using this.

@piernik
Copy link

piernik commented Dec 19, 2014

+1 It's great problem.
What is alternative?

@AlMcKinlay
Copy link

I was looking into this problem (because we have it too).

One solution would be to do an ng-if on the popup. My thought was to have it just do ng-if="isOpen", but that doesn't work as it loses the data stored in the model then, for some reason.

The other option is to create another variable that a user can pass in to say when they want it to be initialised. This would be bound to ng-if and would mean that once it is opened the first time, it is brought into the html and the watchers are assigned.

This would help people who have multiple, as it ends up being a big problem.

However, when trying to do it, I have 5 tests failing. I reckon these are because of the slight performance hit that you get when using ng-if meaning that the popup isn't ready for when it tries to click a date. I'll work on this a bit later tomorrow, but it's at least something that can be done.

@RobJacobs
Copy link
Contributor

@YaManicKill See this pull request for the ng-if implementation: #1915

@AlMcKinlay
Copy link

@RobJacobs Ah, yeah that's exactly what I was working on. Any plans to actually merge it in?

@daric81
Copy link

daric81 commented Mar 12, 2015

+1
Is there any update on this?

@karianna
Copy link
Contributor

@karianna karianna added this to the 0.13.0 milestone Mar 12, 2015
@cpaof
Copy link

cpaof commented Mar 13, 2015

+1
28212 watchers for me ;)

@RobJacobs
Copy link
Contributor

There's simply too many watchers on the templates. Doing a quick look through of the popup.html, the ng-repeat that is building the day/week/cell grid, for each day displayed:

id={{dt.uid}} = 1 watch, 
aria-disabled={{!!dt.disabled}} = 1 watch,
ng-class="{'btn-info': dt.selected}" = 1 watch, 
ng-class="{'text-muted' : dt.secondary = 1 watch, 
{{dt.label}} = 1 watch 

which adds up to 5 watches per cell in the grid.

@cpaof You need to wrap the datepicker popup in an ng-if statement until #1915 get's merged. The plunker mentioned in the first post shows how to do that.

cpaof added a commit to cpaof/bootstrap-bower that referenced this issue Mar 16, 2015
@cpaof
Copy link

cpaof commented Mar 16, 2015

@RobJacobs I decided to create a monkey patched version of the bower library in the end. My use case was the date picker to appear on focus of the input field where the suggested ng-if would not work.

https://github.com/cpaof/bootstrap-bower for anyone that's interested.

@wesleycho
Copy link
Contributor

We are planning on tackling the performance issue with the datepicker once we release 0.13.0.

@karianna karianna modified the milestones: 0.13.1, 0.13.x Apr 12, 2015
@chrisirhc
Copy link
Contributor

Looking into this issue and looks like #2619 adds 40 watchers on the typical use case, even if no custom class was provided.

@wesleycho wesleycho modified the milestones: 0.13.2 (Performance), 0.13.1 (npm) Jul 23, 2015
@wesleycho wesleycho modified the milestones: 0.13.2 (1.4 support), 0.13.3 (Performance) Aug 2, 2015
@wesleycho wesleycho modified the milestones: 0.13.3 (Fixes), 0.13.4 (Performance) Aug 9, 2015
@Foxandxss
Copy link
Contributor

I think we can continue in #3770

chrisirhc added a commit that referenced this issue Dec 30, 2015
- Add new class manipulation helper directive that uses 1 watcher as opposed to watchers for each element with ng-class
- Optimize disabled state
- Optimize class binding

Closes #2613
Closes #3451
Closes #3770
Closes #5065

BREAKING CHANGE: For those using custom templates, the changes result in necessary changes being made to the templates to match the new class changing syntax
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests