Skip to content

Conversation

@mmalerba
Copy link
Contributor

@mmalerba mmalerba commented May 3, 2024

Previously afterRender and afterNextRender allowed the user to pass
a phase to run the callback in as part of the AfterRenderOptions. This
worked, but made it cumbersome to coordinate work between phases.

let size: DOMRect|null = null;

afterRender(() => {
  size = nativeEl.getBoundingClientRect();
}, {phase: AfterRenderPhase.EarlyRead});

afterRender(() => {
  otherNativeEl.style.width = size!.width + 'px';
}, {phase: AfterRenderPhase.Write});

This PR replaces the old phases API with a new one that allows passing a
callback per phase in a single afterRender / afterNextRender call.
The return value of each phase's callback is passed to the next phase
callback that was part of the same afterRender call.

afterRender({
  earlyRead: () => nativeEl.getBoundingClientRect(),
  write: (rect) => {
    otherNativeEl.style.width = rect.width + 'px';
  }
});

This API also retains the ability to pass a single callback, which will
be run in the mixedReadWrite phase.

afterRender(() => {
  // read some stuff ...
  // write some stuff ...
});

@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: docs Related to the documentation labels May 3, 2024
@ngbot ngbot bot added this to the Backlog milestone May 3, 2024
@angular-robot angular-robot bot removed the area: docs Related to the documentation label May 3, 2024
@ngbot ngbot bot removed this from the Backlog milestone May 3, 2024
@mmalerba mmalerba changed the title feat(core): add new signatures of afterRender & afterNextRender feat(core): Redesign the afterRender & afterNextRender phases API May 3, 2024
@mmalerba mmalerba marked this pull request as ready for review May 3, 2024 19:44
@mmalerba mmalerba added target: rc This PR is targeted for the next release-candidate area: core Issues related to the framework runtime labels May 3, 2024
@ngbot ngbot bot added this to the Backlog milestone May 3, 2024
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

I love this API 🥳

Copy link
Contributor

@devknoll devknoll left a comment

Choose a reason for hiding this comment

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

Love this change. Thanks for putting it together!

@mmalerba mmalerba force-pushed the after-render branch 2 times, most recently from b8d5fb7 to 5cbf2f0 Compare May 8, 2024 07:03
@mmalerba mmalerba force-pushed the after-render branch 2 times, most recently from 55744ab to 2e05663 Compare May 18, 2024 05:30
@AndrewKushnir AndrewKushnir added target: minor This PR is targeted for the next minor release and removed target: rc This PR is targeted for the next release-candidate labels May 22, 2024
@mmalerba mmalerba force-pushed the after-render branch 2 times, most recently from b258ee6 to 0c47d6b Compare June 3, 2024 22:26
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@atscott
Copy link
Contributor

atscott commented Jun 4, 2024

Only question I might have would be whether we want to have a migration, which I think should be relatively straightforward.

@mmalerba mmalerba force-pushed the after-render branch 4 times, most recently from 17c7225 to f245f7d Compare June 4, 2024 20:31
@pullapprove pullapprove bot requested review from dylhunn and thePunderWoman June 7, 2024 21:48
mmalerba added 3 commits June 7, 2024 15:16
Previously `afterRender` and `afterNextRender` allowed the user to pass
a phase to run the callback in as part of the `AfterRenderOptions`. This
worked, but made it cumbersome to coordinate work between phases.

```ts
let size: DOMRect|null = null;

afterRender(() => {
  size = nativeEl.getBoundingClientRect();
}, {phase: AfterRenderPhase.EarlyRead});

afterRender(() => {
  otherNativeEl.style.width = size!.width + 'px';
}, {phase: AfterRenderPhase.Write});
```

This PR replaces the old phases API with a new one that allows passing a
callback per phase in a single `afterRender` / `afterNextRender` call.
The return value of each phase's callback is passed to the subsequent
callbacks that were part of that `afterRender` call.

```ts
afterRender({
  earlyRead: () => nativeEl.getBoundingClientRect(),
  write: (rect) => {
    otherNativeEl.style.width = rect.width + 'px';
  }
});
```

This API also retains the ability to pass a single callback, which will
be run in the `mixedReadWrite` phase.

```ts
afterRender(() => {
  // read some stuff ...
  // write some stuff ...
});
```
Adds back the ability to set the phase of an `afterRender` /
`afterNextRender` callback using the `phase` option. However, this API
is now deprecated, and the phase should instead be specified by passing
a spec object rather than a callback function.
Adds an `ng update` migration to move users from using the phase flag
with `afterRender` / `afterNextRender` to passing a spec object instead.
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Jun 10, 2024
@alxhub
Copy link
Member

alxhub commented Jun 10, 2024

This PR was merged into the repository by commit ea3c802.

@alxhub alxhub closed this in a655e46 Jun 10, 2024
alxhub pushed a commit that referenced this pull request Jun 10, 2024
Adds back the ability to set the phase of an `afterRender` /
`afterNextRender` callback using the `phase` option. However, this API
is now deprecated, and the phase should instead be specified by passing
a spec object rather than a callback function.

PR Close #55648
alxhub pushed a commit that referenced this pull request Jun 10, 2024
Adds an `ng update` migration to move users from using the phase flag
with `afterRender` / `afterNextRender` to passing a spec object instead.

PR Close #55648
@spock123
Copy link

Thank you guys, this is a much much more elegant API.

@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 Jul 14, 2024
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 detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants