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

perf: using pure setter with OnChange hook #647

Closed
11 tasks
trotyl opened this issue Nov 30, 2017 · 1 comment
Closed
11 tasks

perf: using pure setter with OnChange hook #647

trotyl opened this issue Nov 30, 2017 · 1 comment
Assignees

Comments

@trotyl
Copy link
Contributor

trotyl commented Nov 30, 2017

Tracking

...

What problem does this feature solve?

Single check

Currently we're using manual change guarding in several places like:

set nzProp(value: Type)
  if (this._prop === value) {
    return;
  }
  this._prop = value;
}

This could introduce extra checking for _prop since Angular itself will check all the inputs (if ngOnChange exists).

Batch update

Currently we're using setter with side-effects, like:

set nzProp1(value: Type)
  this._prop1 = value;
  this.updateStatus();
}

set nzProp2(value: Type)
  this._prop2 = value;
  this.updateStatus();
}

It would lead to redundant update when changing multiple properties:

comp.nzProp1 = newValue1;
comp.nzProp2 = newValue2;

The updateStatus have been called twice, which is unnecessary.

Unified handling

In some components there're recording of life-cycle status, like:

ngOnInit() {
  this._inited = true;
}

set nzProp(value: Type)
  if (this._inited) {
    this.updateStatus();
  }
}

Which leads to extra condition flow (not a real performance problem, but maintenance problem)

Solution

Make setter being pure (and do not use accessor whenever possible) while perform side-effects only in ngOnChange, like:

set nzProp1(value: Type)
  this._prop1 = value;
}

set nzProp2(value: Type)
  this._prop2 = value;
}

ngOnChange(changes: SimpleChanges): void {
  if (changes.nzProp1 || changes.nzProp2) {
    this.updateStatus();
  }
}

Since OnChange will also be called before OnInit, no need for special handling in setup.

Depends on #459.

What does the proposed API look like?

N/A.

@wzhudev
Copy link
Member

wzhudev commented Oct 30, 2018

Closing this cause related pr had been merged.

@wzhudev wzhudev closed this as completed Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants