Skip to content

build: add toggle for OnPush change detection in demo app #3675

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 22, 2017
Merged

build: add toggle for OnPush change detection in demo app #3675

merged 2 commits into from
May 22, 2017
+54 −16

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 19, 2017 •
edited
Loading

Adds a toggle for enabling OnPush change detection in the demo app. This should make it easier to test out components against the different change detection strategies.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 19, 2017
devversion
devversion reviewed Mar 19, 2017
View reviewed changes
src/demo-app/demo-app/demo-app.ts Outdated
@@ -17,8 +26,12 @@ export class Home {}
templateUrl: 'demo-app.html',
styleUrls: ['demo-app.css'],
encapsulation: ViewEncapsulation.None,
changeDetection: isPushChangeDetection ?
ChangeDetectionStrategy.OnPush :
ChangeDetectionStrategy.Default
Copy link
Member

@devversion devversion Mar 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, why don't you store it that way? Looks a bit more clear to me.

let changeDetection = ChangeDetectionStrategy.Default;

try {
  if (localStorage.getItem(changeDetectionKey) {
    changeDetection = ChangeDetectionStrategy.OnPush;
  }
} catch (e) {
  console.error(e);
}

@Component({
  templateUrl: 'XXXX.html',
  changeDetection: changeDetection 
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the boolean is easier for displaying in the UI. AFAIK the detection strategies map to numbers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Yeah I initially thought that setItem(key, changeDetectionStrategy) would work, but it maps to integers.

jelbourn
jelbourn reviewed Mar 20, 2017
View reviewed changes
src/demo-app/demo-app/demo-app.ts Outdated
@@ -17,8 +26,12 @@ export class Home {}
templateUrl: 'demo-app.html',
styleUrls: ['demo-app.css'],
encapsulation: ViewEncapsulation.None,
changeDetection: isPushChangeDetection ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work with AoT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work on the CI which we run against the demo app.

@crisbeto crisbeto force-pushed the demo-app-change-detection-toggle branch from 79a63f8 to ff97b23 Compare May 11, 2017 19:29
@crisbeto
Copy link
Member Author

Re-did this one to avoid AoT issues @jelbourn.

jelbourn
jelbourn approved these changes May 11, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 11, 2017
@kara kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 11, 2017
@kara
Copy link
Contributor

kara commented May 11, 2017

@crisbeto There are some Google test failures related to internal tools not liking localStorage. @jelbourn and I were thinking changing it to window.localStorage may appease them. Can you update?

@kara kara removed the action: merge The PR is ready for merge by the caretaker label May 11, 2017
crisbeto added 2 commits May 12, 2017 10:37
@crisbeto
build: add toggle for OnPush change detection in demo app
698743f 
Adds a toggle for enabling `OnPush` change detection in the demo app. This should make it easier to test out components with the different change detection strategies.
@crisbeto
chore: refer to global variables via the window
c5a46e7
@crisbeto crisbeto force-pushed the demo-app-change-detection-toggle branch from ff97b23 to c5a46e7 Compare May 12, 2017 08:48
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label May 12, 2017
@crisbeto
Copy link
Member Author

Done. Also needed a rebase.

@jelbourn jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 17, 2017
@tinayuangao tinayuangao merged commit 2cf19a3 into angular:master May 22, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion left review comments

@jelbourn jelbourn jelbourn approved these changes

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@crisbeto @kara @jelbourn @devversion @googlebot @tinayuangao