Skip to content

Navigation Menu

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

added logging to the hold method #678

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

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

Conversation

d-stolyar
Copy link

@d-stolyar d-stolyar commented Apr 22, 2021

This PR aims to fix #661

@github-actions github-actions bot added the { } State @rx-angular/state related label Apr 22, 2021
@d-stolyar d-stolyar changed the title added logging to the hold method #661 added logging to the hold method Apr 22, 2021
@hoebbelsB
Copy link
Member

hi @d-stolyar THX for the hotfix! 👍
Could you maybe add a test which covers this? This is already a regression and I'd like it to be the last one for swallowing errors in effects :)

@hoebbelsB
Copy link
Member

oh forgot to mention... If you need any help or don't want to write tests, just ping one of us 👍

@BioPhoton
Copy link
Member

Hi @d-stolyar any updates on your end? We would love to get that change in :)

@github-actions github-actions bot added the 🛂 Test Unit tests, e2e tests, integration tests, test coverage label May 11, 2021
@BioPhoton
Copy link
Member

@distoliar any chance you can bring that in too?

#661 (comment)

@d-stolyar d-stolyar force-pushed the fix/hold-log-error branch from f83f92e to 36a6fa4 Compare May 14, 2021 15:03
Copy link
Member

@edbzn edbzn left a comment

Choose a reason for hiding this comment

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

Hey, I left two comments regarding the DI. Also, I'm wondering why the ErrorHandler is injected as an optional dependency?

libs/state/spec/rx-state.service.spec.ts Show resolved Hide resolved
libs/state/src/lib/rx-state.service.ts Show resolved Hide resolved
@BioPhoton
Copy link
Member

Hi @d-stolyar.
THX for bearing with us! :)

Is the suggested change clear or should we add more information?
Just let me know how we can help.

@d-stolyar
Copy link
Author

d-stolyar commented Jun 17, 2021

@BioPhoton
#678 (comment)

Copy link
Member

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

Lets merge it finaly! 😁🚀

@hoebbelsB
Copy link
Member

@d-stolyar can u please merge master into your branch? :)

Comment on lines +593 to +594
const errorHandler = inject(ErrorHandler, InjectFlags.Optional);
errorHandler?.handleError(e);
Copy link
Member

Choose a reason for hiding this comment

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

Last detail, why considering the ErrorHandler as optional? It should always be defined.

Suggested change
const errorHandler = inject(ErrorHandler, InjectFlags.Optional);
errorHandler?.handleError(e);
const errorHandler = inject(ErrorHandler);
errorHandler.handleError(e);

Copy link
Member

@arturovt arturovt Aug 2, 2021

Choose a reason for hiding this comment

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

He has probably added it because it throws inject() must be called from an injection context since there's no injector on the instruction stack.

Your recommendation with this one is correct:

constructor(private errorHandler: ErrorHandler) {}

Since the stable version is not released yet I think it's ok to have breaking changes. We can have a migration script for that.

Copy link
Member

Choose a reason for hiding this comment

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

We can create a migration script, but honestly, this looks bad:

  constructor(private errorHandler: ErrorHandler) {
    super(this.errorHandler);
  }

I would prefer keeping the InjectFlags.Optional hack to hide this noise from the user.

Copy link
Member

@arturovt arturovt Aug 2, 2021

Choose a reason for hiding this comment

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

I've just checked that code and it seems like it's working only inside tests for now because the injector is explicitly set there. E.g. this doesn't work even with inject flags:

@Component({
  selector: 'app-root',
  template: '<button (click)="onClick()">Click me</button>',
})
export class AppComponent {
  onClick(): void {
    console.log(inject(ErrorHandler, InjectFlags.Optional));
  }
}

That would respect inject flags if there's any injector on the stack:

function injectInjectorOnly(token, flags = InjectFlags.Default) {
    if (_currentInjector === undefined) {
        throw new Error(`inject() must be called from an injection context`);
    }
}
function ɵɵinject(token, flags = InjectFlags.Default) {
    // The `getInjectImplementation` returns `undefined` here
    return (getInjectImplementation() || injectInjectorOnly)(resolveForwardRef(token), flags);
}

I'm wondering if it can be moved to the constructor but it also has to be wrapped into try-catch to prevent the error if there is no current injector:

export class ... {
  private errorHandler: ErrorHandler | null;

  constructor() {
    try {
      this.errorHandler = inject(ErrorHandler);
    } catch {
      this.errorHandler = null;
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it can be moved to the constructor but it also has to be wrapped into try-catch to prevent the error if there is no current injector:

export class ... {
  private errorHandler: ErrorHandler | null;

  constructor() {
    try {
      this.errorHandler = inject(ErrorHandler);
    } catch {
      this.errorHandler = null;
    }
  }

this sounds like the only valid solution for me since the approach with extending from RxState is pretty common.

Copy link
Author

@d-stolyar d-stolyar Aug 4, 2021

Choose a reason for hiding this comment

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

How about this? @hoebbelsB

class ... {
  private errorHandler: ErrorHandler

  constructor() {
    try {
      this.errorHandler = inject(ErrorHandler)
    } catch {
      const injector = Injector.create({
        providers: [{ provide: ErrorHandler, deps: [] }]
      });
      this.errorHandler = injector.get(ErrorHandler);
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

This will create ErrorHandlers per each RxState instance and will have nothing to do with the global error handler (the one provided by a user).

Copy link
Member

Choose a reason for hiding this comment

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

To conclude, we should try this approach and decide:

export class ... {
  private errorHandler: ErrorHandler | null;

  constructor() {
    try {
      this.errorHandler = inject(ErrorHandler);
    } catch {
      this.errorHandler = null;
    }
  }

@d-stolyar d-stolyar changed the base branch from master to add-rx-effects August 3, 2021 15:29
@d-stolyar d-stolyar changed the base branch from add-rx-effects to master August 3, 2021 15:29
@d-stolyar d-stolyar force-pushed the fix/hold-log-error branch from 2bd93d5 to 8c3a9c1 Compare August 4, 2021 11:12
@nx-cloud
Copy link

nx-cloud bot commented Aug 4, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit be81e89.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core team meeting { } 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.

Error logging in hold streams
7 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.