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

Allow app.select to not abort on error #13735

Closed
1 task done
RocheVal opened this issue Jul 1, 2024 · 4 comments
Closed
1 task done

Allow app.select to not abort on error #13735

RocheVal opened this issue Jul 1, 2024 · 4 comments
Labels
needs triage This issue has not been looked into type: enhancement 🐺

Comments

@RocheVal
Copy link

RocheVal commented Jul 1, 2024

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

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: #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
Copy link
Member

micalevisk commented Jul 1, 2024

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
Copy link
Author

RocheVal commented Jul 2, 2024

Yes I think this is better.

@micalevisk
Copy link
Member

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)

@github-project-automation github-project-automation bot moved this to 🤔 I'll investigate later in My contributions to NestJS framework 😻 Jul 7, 2024
@micalevisk micalevisk moved this from 🤔 I'll investigate later to 🕒 I'm trying... in My contributions to NestJS framework 😻 Sep 22, 2024
@micalevisk micalevisk moved this from 🕒 I'm trying... to 🤔 I'll investigate later in My contributions to NestJS framework 😻 Sep 22, 2024
@kamilmysliwiec
Copy link
Member

Let's track this here #14113

@github-project-automation github-project-automation bot moved this from 🤔 I'll investigate later to 😔 abandoned in My contributions to NestJS framework 😻 Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: enhancement 🐺
Projects
None yet
Development

No branches or pull requests

3 participants