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

fix(state): handle error emission in connect using ErrorHandler #1553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

edbzn
Copy link
Member

@edbzn edbzn commented Apr 26, 2023

It's potentially a breaking change as with these changes instantiating RxState outside of Angular injection context will raise the following error:

NG0203: inject() must be called from an injection context such as a constructor, a factory function, a field initializer, or a function used with `EnvironmentInjector#runInContext`. Find more at https://angular.io/errors/NG0203

However, I don't see any good reasons to instantiate RxState outside of a field initializer or a factory function.

Fixes #1536

@edbzn edbzn requested a review from hoebbelsB April 26, 2023 08:37
@nx-cloud
Copy link

nx-cloud bot commented Apr 26, 2023

☁️ Nx Cloud Report

Attention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here.

CI is running/has finished running commands for commit fa72532. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


🟥 Failed Commands
nx affected:test --parallel --max-parallel=1 --max-workers=2 --ci --code-coverage
✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@github-actions github-actions bot added { } State @rx-angular/state related 🛂 Test Unit tests, e2e tests, integration tests, test coverage labels Apr 26, 2023
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #1553 (246f7e6) into main (0a599db) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 246f7e6 differs from pull request most recent head fa72532. Consider uploading reports for the commit fa72532 to get more accurate results

@@            Coverage Diff             @@
##             main    #1553      +/-   ##
==========================================
+ Coverage   81.60%   81.62%   +0.01%     
==========================================
  Files         104      104              
  Lines        2229     2231       +2     
  Branches      407      407              
==========================================
+ Hits         1819     1821       +2     
  Misses        332      332              
  Partials       78       78              
Flag Coverage Δ
state 90.36% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tate/selections/src/lib/accumulation-observable.ts 100.00% <100.00%> (ø)
libs/state/src/lib/rx-state.service.ts 100.00% <100.00%> (ø)

@edbzn edbzn marked this pull request as ready for review May 7, 2023 17:06
Copy link
Member

@hoebbelsB hoebbelsB left a comment

Choose a reason for hiding this comment

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

hello @edbzn thx for the PR :)

I'm not even sure if my comment is actually correct, but I guess we will have a problem with the execution context. If not, please ignore me and I'm going to approve. Sorry, had no time to actually check that, it just came to my mind ^^

Comment on lines 56 to 59
private handleError = inject(ErrorHandler).handleError;
private accumulator = createAccumulationObservable<T>({
handleError: this.handleError,
});
Copy link
Member

Choose a reason for hiding this comment

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

We have to make sure the correct execution context is provided. Not sure if the current solution works if you try to access fields inside the handleError function.

e.g.

class ErrorHandler {
  handleError() {
    if (this.env === 'prod') {
       // do something in prod
    }
  }
}

could we create an test case for that?

Suggested change
private handleError = inject(ErrorHandler).handleError;
private accumulator = createAccumulationObservable<T>({
handleError: this.handleError,
});
private errorHandler = inject(ErrorHandler);
private accumulator = createAccumulationObservable<T>({
handleError: this.erroHandler.handleError.bind(this.errorHandler),
});

Copy link
Member Author

@edbzn edbzn May 25, 2023

Choose a reason for hiding this comment

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

I pushed the changes, but the test is failing, I'm not sure to get why. If you have an idea @hoebbelsB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
{ } State @rx-angular/state related 🛂 Test Unit tests, e2e tests, integration tests, test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle error from Observable in rxState.connect()
2 participants