-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
base: main
Are you sure you want to change the base?
Conversation
hi @d-stolyar THX for the hotfix! 👍 |
oh forgot to mention... If you need any help or don't want to write tests, just ping one of us 👍 |
Hi @d-stolyar any updates on your end? We would love to get that change in :) |
@distoliar any chance you can bring that in too? |
f83f92e
to
36a6fa4
Compare
There was a problem hiding this 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?
Hi @d-stolyar. Is the suggested change clear or should we add more information? |
There was a problem hiding this 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! 😁🚀
@d-stolyar can u please merge master into your branch? :) |
const errorHandler = inject(ErrorHandler, InjectFlags.Optional); | ||
errorHandler?.handleError(e); |
There was a problem hiding this comment.
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.
const errorHandler = inject(ErrorHandler, InjectFlags.Optional); | |
errorHandler?.handleError(e); | |
const errorHandler = inject(ErrorHandler); | |
errorHandler.handleError(e); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create ErrorHandler
s per each RxState instance and will have nothing to do with the global error handler (the one provided by a user).
There was a problem hiding this comment.
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;
}
}
2bd93d5
to
8c3a9c1
Compare
Nx Cloud ReportWe didn't find any information for the current pull request with the commit be81e89. Check the Nx Cloud Github Integration documentation for more information. Sent with 💌 from NxCloud. |
This PR aims to fix #661