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

Nest should not swallow exceptions in NestFactoryStatic.initialize #5162

Closed
larose opened this issue Jul 29, 2020 · 10 comments
Closed

Nest should not swallow exceptions in NestFactoryStatic.initialize #5162

larose opened this issue Jul 29, 2020 · 10 comments

Comments

@larose
Copy link

larose commented Jul 29, 2020

If an exception is thrown in:

try {
this.logger.log(MESSAGES.APPLICATION_START);
await ExceptionsZone.asyncRun(async () => {
await dependenciesScanner.scan(module);
await instanceLoader.createInstancesOfDependencies();
dependenciesScanner.applyApplicationProviders();
});
} catch (e) {
process.abort();
}

I don't think there is a way to capture it outside of nest.

Expected behavior

I would expect the exception to bubble up so I can capture it. For example, I'd like to send it to Sentry.

@larose larose added the needs triage This issue has not been looked into label Jul 29, 2020
@kamilmysliwiec
Copy link
Member

Can you please share a minimal repository that shows what you want to accomplish and why this snippet of code is a blocker for you?

@kamilmysliwiec kamilmysliwiec added needs clarification and removed needs triage This issue has not been looked into labels Jul 30, 2020
@larose
Copy link
Author

larose commented Jul 30, 2020

Thanks @kamilmysliwiec, I've created a minimal repository: https://github.com/larose/nest-error-swallow-on-init.

It's not a blocker per say as there are ways to detect failures during Nest initialization outside of the process, but this is not convenient. In my case, my Sentry dashboard will show no errors, even though my Nest app doesn't start.

Nest should not catch all errors, but let them bubble up instead so the client code can decide what to do with them.

@larose
Copy link
Author

larose commented Aug 18, 2020

@kamilmysliwiec, I see that this issue still has the "needs clarification" label. Did my comment above clarified things?

@kamilmysliwiec
Copy link
Member

I think we should add an option named abortOnError that could be disabled on-demand to avoid breaking changes. Example:

NestFactory.create(AppModule, { abortOnError: false })

Would you like to create a PR for this issue?

@princechauhan1992
Copy link
Contributor

Isn't a better way of doing this would be to provide an interface NestExceptionsHandler an implementation of which can be passed into the options in nestfactory.create() and that exceptionhandler can be used in place of the default exception handler in exceptions-zone.js
export class ExceptionHandler implements NestExceptionHandler { handle(error) { // your custom error logic} } }
and then
await NestFactory.create(MainModule, {exceptionHandler: new ExceptionHandler()})
and then it can be used in exception-zone.js before calling teardown()
catch (e) { this.exceptionHandler.handle(e); teardown }. . Will that be a breaking change? @kamilmysliwiec

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Aug 19, 2020

@princechauhan1992 what I've proposed (abortOnError) should actually cover the same set of use-cases. I mean, obviously when someone would enable abortOnError: true, the error would be re-thrown (instead of calling process.abort()), and so it would be possible to wrap the NestFactory.create() method with the try..catch blocks to catch and process the exception.

@princechauhan1992
Copy link
Contributor

Sounds good @kamilmysliwiec. @larose @kamilmysliwiec I would like to create a pr for this, if you allow me.

@kamilmysliwiec
Copy link
Member

Sure thing! PRs are more than welcome

@larose
Copy link
Author

larose commented Aug 19, 2020

Sounds good @kamilmysliwiec. @larose @kamilmysliwiec I would like to create a pr for this, if you allow me.

👍 thanks!

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Nov 2, 2020

PR #5327 is now merged and released as 7.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants