-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
feat(core): add hint to unknown dependency error msg #10558
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
Conversation
2c29360
to
a50a846
Compare
a50a846
to
80a60cf
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.
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
39217bf
to
77d13a5
Compare
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 |
77d13a5
to
65a705c
Compare
@jmcdo29 btw what do you think of this message instead: |
@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? |
65a705c
to
d3a025c
Compare
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Sometimes people wrongly put things that aren't modules inside
imports
array, which leads to errors such as:in this example we have a class annotated with
@WebSocketGateway()
placed atimports
array ofEventsModule
I found that sometimes people didn't understand that the error message tells that
EventsGateway
is being treated as a nestjs moduleWhat is the new behavior?
In the scenario described above, the error message will be:
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:
so people won't complain on what it's a 'valid module'
Does this PR introduce a breaking change?