Skip to content

fix(core): remove rejectErrors option encourages uncaught exceptions#60397

Closed
atscott wants to merge 1 commit intoangular:mainfrom
atscott:removeRejectErrors
Closed

fix(core): remove rejectErrors option encourages uncaught exceptions#60397
atscott wants to merge 1 commit intoangular:mainfrom
atscott:removeRejectErrors

Conversation

@atscott
Copy link
Copy Markdown
Contributor

@atscott atscott commented Mar 14, 2025

This commit removes the previously added rejectErrors option from toSignal which was meant to create similar behavior to what happens with unhandled errors in AsyncPipe.

This option promotes unhandled exceptions and attaches behaviors that we do not believe are desirable as an option offered by the framework:

  • Unhandled errors that are thrown lose the context of where the error ocurred. Throwing the error where the signal is read allows error handling to be performed at the signal's usage location
  • With this feature, the value of the signal remains set to the initial value or the previous successful value coming out of the Observable. We do not feel this is appropriate implicit behavior but should be an explicit choice by the application. Signals are built to represent state. When an observable stream is converted to a stateful representation, there should be a choice made about what state should be presented when an error occurs
  • If an error occurs but the signal value is never read in its error state, this is not an application state error that should result in an unhandled exception.
  • While Angular does not have error boundaries today, there is likely to be investigation in the near future to address increased risk of errors thrown from signals such as this in templates. Without pre-designing any specifics, it is possible that this type of feature would require the error to be thrown from the signal. We are subsequently not prepared to commit to stabilizing the toSignal API with the rejectErrors option and its current behavior.

Developers that desire similar behavior to rejectErrors have several options, the simplest of which would be something similar to toSignal(myStream.pipe(catchError(() => EMPTY))).

@atscott atscott added the target: major This PR is targeted for the next major release label Mar 14, 2025
@atscott atscott requested a review from alxhub March 14, 2025 18:16
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Mar 14, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 14, 2025
This commit removes the previously added `rejectErrors` option from
`toSignal` which was meant to create similar behavior to what happens
with unhandled errors in `AsyncPipe`.

This option promotes unhandled exceptions and attaches behaviors that we
do not believe are desirable as an option offered by the framework:

* Unhandled errors that are thrown lose the context of where the error
  ocurred. Throwing the error where the signal is read allows error
  handling to be performed at the signal's usage location
* With this feature, the value of the signal remains set to the initial
  value or the previous successful value coming out of the `Observable`.
  We do not feel this is appropriate implicit behavior but should be an
  explicit choice by the application. Signals are built to represent
  state. When an observable stream is converted to a stateful
  representation, there should be a choice made about what state should
  be presented when an error occurs
* If an error occurs but the signal value is never read in its error
  state, this is not an application state error that should result in an
  unhandled exception.
* While Angular does not have error boundaries today, there is likely to
  be investigation in the near future to address increased risk of
  errors thrown from signals such as this in templates. Without
  pre-designing any specifics, it is possible that this type of feature
  would require the error to be thrown from the signal. We are subsequently
  not prepared to commit to stabilizing the `toSignal` API with the
  `rejectErrors` option and its current behavior.

Developers that desire similar behavior to `rejectErrors` have several
options, the simplest of which would be something similar to
`toSignal(myStream.pipe(catchError(() => EMPTY)))`.
Copy link
Copy Markdown
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from alxhub March 14, 2025 20:09
Copy link
Copy Markdown
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Should we note this as a breaking change? (or do I miss something?)

it was seemingly added in v17 via 7bb3ffb

@pullapprove pullapprove bot requested a review from crisbeto March 15, 2025 12:02
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Mar 15, 2025

@devversion sure, I can add it for good measure.

There were only two uses internally and API is still in dev preview so technically doesn’t require it. I also felt like because it’d break at compile time rather than a behavioral change you might not otherwise notice, it’d be obvious enough.

@devversion
Copy link
Copy Markdown
Member

yeah, I was under impression that typically we still mark those as breaking changes (since they are breaking); but we are just allowed to make them for dev-preview code even in e.g. a minor

Copy link
Copy Markdown
Member

@alxhub alxhub 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

@pkozlowski-opensource pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Mar 17, 2025
@pkozlowski-opensource
Copy link
Copy Markdown
Member

This PR was merged into the repository by commit 48974c3.

The changes were merged into the following branches: main

@angular-automatic-lock-bot
Copy link
Copy Markdown

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 Apr 18, 2025
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 target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants