10000 fix(core): markDirty() should only mark flags when really scheduling tick by JiaLiPassion · Pull Request #39316 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

fix(core): markDirty() should only mark flags when really scheduling tick #39316

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
Closed
wants to merge 1 commit into from
Closed

fix(core): markDirty() should only mark flags when really scheduling tick #39316

wants to merge 1 commit into from

Conversation

JiaLiPassion
Copy link
Contributor

Close #39296

Fix an issue that markDirty() will not trigger change detection.

The case is for example we have the following component.

export class AppComponent implements OnInit {
  constructor(private router: Router) {}

  ngOnInit() {
    this.router.events
      .pipe(filter((e) => e instanceof NavigationEnd))
      .subscribe(() => ɵmarkDirty(this));
  }
}

export class CounterComponent implements OnInit, OnDestroy {
  ngOnInit() {
    this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => {
      this.count = count;
      ɵmarkDirty(this);
    });
  }

Then the app navigate from AppComponent to CounterComponent,
so there are 2 markDirty() call at in a row.

The 1st call is from AppComponent when router changed, the
2nd call is from CounterComponent.ngOnInit().

And the markDirty()->scheduleTick() code look like this

function scheduleTick(rootContext, flags) {
    const nothingScheduled = rootContext.flags === 0 /* Empty */;
    rootContext.flags |= flags;
    if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
      rootContext.schedule(() => {
	...
        if (rootContext.flags & RootContextFlags.DetectChanges)
          rootContext.flags &= ~RootContextFlags.DetectChanges;
          tickContext();

        rootContext.clean = _CLEAN_PROMISE;
        ...
      });

So in this case, the 1st markDirty() will

  1. set rootContext.flags = 1
  2. before tickContext(), reset rootContext.flags = 0
  3. inside tickContext(), it will call CounterComponent.ngOnint(),
    so the 2nd markDirty() is called.
  4. and the 2nd scheduleTick is called, nothingScheduled is true,
    but rootContext.clean is not _CLEAN_PROMISE yet, since the 1st markDirty tick
    is still running.
  5. So nowhere will reset the rootContext.flags.
  6. then in the future, any other markDirty() call will not trigger the tick, since
    nothingScheduled is always false.

So nothingScheduled means no tick is scheduled, rootContext.clean === _CLEAN_PROMISE
means no tick is running.
So we should set the flags to rootContext only when no tick is scheudled or running.

@JiaLiPassion JiaLiPassion added the area: core Issues related to the framework runtime label Oct 17, 2020
@JiaLiPassion JiaLiPassion requested a review from mhevery October 17, 2020 14:09
@ngbot ngbot bot added this to the needsTriage milestone Oct 17, 2020
@google-cla google-cla bot added the cla: yes label Oct 17, 2020
@mhevery mhevery self-assigned this Oct 19, 2020
@JiaLiPassion JiaLiPassion force-pushed the markdirty-coaleascing branch from 6184918 to 17fcefb Compare October 19, 2020 23:33
@JiaLiPassion JiaLiPassion requested a review from mhevery October 19, 2020 23:34
@JiaLiPassion JiaLiPassion force-pushed the markdirty-coaleascing branch 2 times, most recently from 9c9d007 to cb6c1b8 Compare October 19, 2020 23:59
@JiaLiPassion
fix(core): markDirty() should only mark flags when really scheduling …
1a82495 
…tick.

Close angular#39296

Fix an issue that `markDirty()` will not trigger change detection.

The case is for example we have the following component.

```
export class AppComponent implements OnInit {
  constructor(private router: Router) {}

  ngOnInit() {
    this.router.events
      .pipe(filter((e) => e instanceof NavigationEnd))
      .subscribe(() => ɵmarkDirty(this));
  }
}

export class CounterComponent implements OnInit, OnDestroy {
  ngOnInit() {
    this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => {
      this.count = count;
      ɵmarkDirty(this);
    });
  }
```

Then the app navigate from `AppComponent` to `CounterComponent`,
so there are 2 `markDirty()` call at in a row.

The `1st` call is from `AppComponent` when router changed, the
`2nd` call is from `CounterComponent.ngOnInit()`.

And the `markDirty()->scheduleTick()` code look like this

```
function scheduleTick(rootContext, flags) {
    const nothingScheduled = rootContext.flags === 0 /* Empty */;
    rootContext.flags |= flags;
    if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
      rootContext.schedule(() => {
	...
        if (rootContext.flags & RootContextFlags.DetectChanges)
          rootContext.flags &= ~RootContextFlags.DetectChanges;
          tickContext();

        rootContext.clean = _CLEAN_PROMISE;
        ...
      });
```

So in this case, the `1st` markDirty() will
1. set rootContext.flags = 1
2. before `tickContext()`, reset rootContext.flags = 0
3. inside `tickContext()`, it will call `CounterComponent.ngOnint()`,
   so the `2nd` markDirty() is called.
4. and the `2nd` scheduleTick is called, `nothingScheduled` is true,
   but rootContext.clean is not `_CLEAN_PROMISE` yet, since the `1st` markDirty tick
   is still running.
5. So nowhere will reset the `rootContext.flags`.
6. then in the future, any other `markDirty()` call will not trigger the tick, since
   `nothingScheduled` is always false.

So `nothingScheduled` means no tick is scheduled, `rootContext.clean === _CLEAN_PROMISE`
means no tick is running.
So we should set the flags to `rootContext` only when `no tick is scheudled or running`.
@JiaLiPassion JiaLiPassion force-pushed the markdirty-coaleascing branch from cb6c1b8 to 1a82495 Compare October 20, 2020 00:01
@mhevery
Copy link
Contributor
mhevery commented Oct 21, 2020

presubmit

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release cla: yes and removed cla: yes labels Oct 29, 2020
josephperrott pushed a commit that referenced this pull request Oct 29, 2020
@JiaLiPassion @josephperrott
fix(core): markDirty() should only mark flags when really scheduling …
8c82106 
…tick. (#39316)

Close #39296

Fix an issue that `markDirty()` will not trigger change detection.

The case is for example we have the following component.

```
export class AppComponent implements OnInit {
  constructor(private router: Router) {}

  ngOnInit() {
    this.router.events
      .pipe(filter((e) => e instanceof NavigationEnd))
      .subscribe(() => ɵmarkDirty(this));
  }
}

export class CounterComponent implements OnInit, OnDestroy {
  ngOnInit() {
    this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => {
      this.count = count;
      ɵmarkDirty(this);
    });
  }
```

Then the app navigate from `AppComponent` to `CounterComponent`,
so there are 2 `markDirty()` call at in a row.

The `1st` call is from `AppComponent` when router changed, the
`2nd` call is from `CounterComponent.ngOnInit()`.

And the `markDirty()->scheduleTick()` code look like this

```
function scheduleTick(rootContext, flags) {
    const nothingScheduled = rootContext.flags === 0 /* Empty */;
    rootContext.flags |= flags;
    if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
      rootContext.schedule(() => {
	...
        if (rootContext.flags & RootContextFlags.DetectChanges)
          rootContext.flags &= ~RootContextFlags.DetectChanges;
          tickContext();

        rootContext.clean = _CLEAN_PROMISE;
        ...
      });
```

So in this case, the `1st` markDirty() will
1. set rootContext.flags = 1
2. before `tickContext()`, reset rootContext.flags = 0
3. inside `tickContext()`, it will call `CounterComponent.ngOnint()`,
   so the `2nd` markDirty() is called.
4. and the `2nd` scheduleTick is called, `nothingScheduled` is true,
   but rootContext.clean is not `_CLEAN_PROMISE` yet, since the `1st` markDirty tick
   is still running.
5. So nowhere will reset the `rootContext.flags`.
6. then in the future, any other `markDirty()` call will not trigger the tick, since
   `nothingScheduled` is always false.

So `nothingScheduled` means no tick is scheduled, `rootContext.clean === _CLEAN_PROMISE`
means no tick is running.
So we should set the flags to `rootContext` only when `no tick is scheudled or running`.

PR Close #39316
josephperrott pushed a commit that referenced this pull request Oct 29, 2020
@JiaLiPassion @josephperrott
fix(core): markDirty() should only mark flags when really scheduling …
3b6497b 
…tick. (#39316)

Close #39296

Fix an issue that `markDirty()` will not trigger change detection.

The case is for example we have the following component.

```
export class AppComponent implements OnInit {
  constructor(private router: Router) {}

  ngOnInit() {
    this.router.events
      .pipe(filter((e) => e instanceof NavigationEnd))
      .subscribe(() => ɵmarkDirty(this));
  }
}

export class CounterComponent implements OnInit, OnDestroy {
  ngOnInit() {
    this.countSubject.pipe(takeUntil(this.destroy)).subscribe((count) => {
      this.count = count;
      ɵmarkDirty(this);
    });
  }
```

Then the app navigate from `AppComponent` to `CounterComponent`,
so there are 2 `markDirty()` call at in a row.

The `1st` call is from `AppComponent` when router changed, the
`2nd` call is from `CounterComponent.ngOnInit()`.

And the `markDirty()->scheduleTick()` code look like this

```
function scheduleTick(rootContext, flags) {
    const nothingScheduled = rootContext.flags === 0 /* Empty */;
    rootContext.flags |= flags;
    if (nothingScheduled && rootContext.clean == _CLEAN_PROMISE) {
      rootContext.schedule(() => {
	...
        if (rootContext.flags & RootContextFlags.DetectChanges)
          rootContext.flags &= ~RootContextFlags.DetectChanges;
          tickContext();

        rootContext.clean = _CLEAN_PROMISE;
        ...
      });
```

So in this case, the `1st` markDirty() will
1. set rootContext.flags = 1
2. before `tickContext()`, reset rootContext.flags = 0
3. inside `tickContext()`, it will call `CounterComponent.ngOnint()`,
   so the `2nd` markDirty() is called.
4. and the `2nd` scheduleTick is called, `nothingScheduled` is true,
   but rootContext.clean is not `_CLEAN_PROMISE` yet, since the `1st` markDirty tick
   is still running.
5. So nowhere will reset the `rootContext.flags`.
6. then in the future, any other `markDirty()` call will not trigger the tick, since
   `nothingScheduled` is always false.

So `nothingScheduled` means no tick is scheduled, `rootContext.clean === _CLEAN_PROMISE`
means no tick is running.
So we should set the flags to `rootContext` only when `no tick is scheudled or running`.

PR Close #39316
@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 Nov 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

Assignees

@mhevery mhevery

Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Change Detection Not Working with Routing in Zone-Less Application
2 participants
@JiaLiPassion @mhevery
0