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

feat(core): add hint to unknown dependency error msg #10558

Merged

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Nov 12, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Sometimes people wrongly put things that aren't modules inside imports array, which leads to errors such as:

image

in this example we have a class annotated with @WebSocketGateway() placed at imports array of EventsModule

I found that sometimes people didn't understand that the error message tells that EventsGateway is being treated as a nestjs module

What is the new behavior?

In the scenario described above, the error message will be:

image

I hope that with this minor change people we now understand why they got that error faster.

I'm a bit concerd on the proposed message tho. Another suggestion would be:

- If ${dependencyName} is a provider, is it part of the current module Module?
- If ${dependencyName} is exported from a separate @Module, is that module imported within the module ${moduleName} 

so people won't complain on what it's a 'valid module'

Does this PR introduce a breaking change?

  • Yes
  • No

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

Love the additional info in the error message! Just wondering about if undefined would ever be a valid value, as getModuleName returns 'current' as a fallback if no name is found

@micalevisk micalevisk force-pushed the feat/add-solution-on-unknown-dep branch 2 times, most recently from 39217bf to 77d13a5 Compare November 15, 2022 03:30
@micalevisk
Copy link
Member Author

good catch! I didn't notice that. As we don't want to prompt out 'current is a valid NestJS module', we need to check if we have the module name to display

@micalevisk micalevisk force-pushed the feat/add-solution-on-unknown-dep branch from 77d13a5 to 65a705c Compare November 15, 2022 03:35
@micalevisk
Copy link
Member Author

@jmcdo29 btw what do you think of this message instead:
Is <module> a valid NestJS module? If not, it must not appear in the "imports" array

@jmcdo29
Copy link
Member

jmcdo29 commented Nov 15, 2022

@jmcdo29 btw what do you think of this message instead: Is <module> a valid NestJS module? If not, it must not appear in the "imports" array

@micalevisk I think I'd rather we link to the modules documentation page. Maybe even add a link to the Common Errors docs too at the end of the message and make extra mentions there as needed.

@kamilmysliwiec thoughts on linking to the docs in these cases?

@micalevisk micalevisk force-pushed the feat/add-solution-on-unknown-dep branch from 65a705c to d3a025c Compare November 15, 2022 04:10
@micalevisk
Copy link
Member Author

well, links are clickable in my terminal at least :p

image

@kamilmysliwiec kamilmysliwiec merged commit 220b098 into nestjs:master Nov 18, 2022
@kamilmysliwiec
Copy link
Member

LGTM

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

Successfully merging this pull request may close these issues.

None yet

3 participants