Skip to content
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.

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

Breaking Change introduced with change to throwError usage in #3312 #3702

Closed
1 of 2 tasks
markonyango opened this issue Dec 7, 2022 · 0 comments · Fixed by #3723
Closed
1 of 2 tasks

Breaking Change introduced with change to throwError usage in #3312 #3702

markonyango opened this issue Dec 7, 2022 · 0 comments · Fixed by #3723
Labels
Accepting PRs bug Good First Issue Good issue for first-time contributor

Comments

@markonyango
Copy link

markonyango commented Dec 7, 2022

Which @ngrx/* package(s) are the source of the bug?

component-store, component, data, effects

Minimal reproduction of the bug/regression with instructions

https://ngrx-seed-jt4hej.stackblitz.io

Switch between rxjs version 6.5.5 and 7.0.0 to see the issue.

Minimal reproduction of the bug/regression with instructions

Prior to refactoring (3a9e1da) usage of throwError the error handler function from e.g. a call to service.getByKey('xxx') would yield the DataServiceError and by accessing the error property the underlying HttpErrorResponse object.

Since throwError now also supports passing functions as parameter (ReactiveX/rxjs@dad270a), a refactoring in several ngrx modules (data, component-store, effect) was undertaken to change instances of throwError(error) to throwError(() => error.
While this works with any rxjs versions 7+ this breaks compatibility with version rxjs 6.5.x as throwError due to the anonymous function (i.e. () => error) not being called prior to returning the error.

grafik

ngrx/{data, component-store, effect} v14+ all have peer dependencies to rxjs in the form of "rxjs": "^6.5.3 || ^7.5.0" indicating both version in the minor and major semver range to be compatible. Not updating rxjs will however break code relying on error to not be a function but the error instance itself.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

Ngrx/data@14+
Ngrx/component-store@14+
Ngrx/component@14+
Ngrx/effect@14+

Other information

I would be willing to submit a PR to fix this issue but would argue that removing rxjs@6.5x as peer dependency should communicate this breaking change to every developer much better.

HOWEVER, that might retro-actively breaking current implementations. I'd like to discuss best viable options prior to submitting a PR.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@timdeschryver timdeschryver added bug Accepting PRs Good First Issue Good issue for first-time contributor labels Dec 21, 2022
markostanimirovic added a commit to markostanimirovic/platform that referenced this issue Dec 21, 2022
markonyango added a commit to markonyango/platform that referenced this issue Dec 30, 2022
This reverts commit <3a9e1da4ad78ceedbf00a2088613848e8bbce353>.

Closes ngrx#3702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs bug Good First Issue Good issue for first-time contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants