nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
65.98k stars 7.48k forks source link

Allow `app.select` to not abort on error #13735

Open RocheVal opened 3 weeks ago

RocheVal commented 3 weeks ago

Is there an existing issue that is already proposing this?

Is your feature request related to a problem? Please describe it

When using app.select to get a module than have been loaded dynamically if the module is not found the app will crash and it's not possible to catch the error to prevent the crash.

Describe the solution you'd like

Add a new boolean arg abortOnError to app.select that allow to not "abort on error" and catch the error. By default abortOnError will be true to keep the current behavior.

Cf this issue for more details: https://github.com/nestjs/nest/issues/13033

Teachability, documentation, adoption, migration strategy

Users can pass the new arg to false to disable the "abort" behavior.

With true as default value there are no retro compatibility problems.

What is the motivation / use case for changing the behavior?

I have some modules that are loaded dynamically based on different parameters/conditions. But some modules will need some specific operations. So I select a module, if it's loaded I will run the specific operations, if it's not I will ignore it and do nothing. Right now if the module is not loaded the app crash.

micalevisk commented 3 weeks ago

By default abortOnError will be true to keep the current behavior.

I'd say that we should align it with the abortOnError of NestFactory instead.

So if we supply abortOnError: true to NestFactory, app.select(Bar) will use abortOnError:true as well, which is the current behavior.
If we supply abortOnError:false to NestFactory, app.select(Bar) will use abortOnError: false as well.
Then we can fine-tuning each app.select call if we want to by calling app.select(Bar, { abortOnError: true })

I believe this is better than having to coordinate the two behaviors in several places (imagine using app.select in multiple places). Although could be confusing if we have app.select(Bar, { abortOnError: true }) and app.select(Qux) because it won't be that clear what is the default for the latter

What do you think?

RocheVal commented 3 weeks ago

Yes I think this is better.

micalevisk commented 2 weeks ago

and for completeness, I'd say that we should support the same for app.get() errors as well. As of now, we cannot handle exceptions on them when having abortOnError:true (the default)