Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

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

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterangular/angular:masterfrom
JiaLiPassion:markdirty-coaleascingJiaLiPassion/angular:markdirty-coaleascingCopy head branch name to clipboard
Closed

fix(core): markDirty() should only mark flags when really scheduling tick#39316
JiaLiPassion wants to merge 1 commit intoangular:masterangular/angular:masterfrom
JiaLiPassion:markdirty-coaleascingJiaLiPassion/angular:markdirty-coaleascingCopy head branch name to clipboard

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
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
@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
…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
…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
…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.

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

Development

Successfully merging this pull request may close these issues.

Change Detection Not Working with Routing in Zone-Less Application

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.