feat: add warning message for the info command #2396
Conversation
actions/info.action.ts
Outdated
| ): NestDependencyWarnings { | ||
| const unsortedWarnings: NestDependencyWarnings = nestDependencies.reduce<NestDependencyWarnings>( | ||
| (acc, { name, fullName, value }) => { | ||
| const [major, minor] = value.split('.').map(parseFloat); |
There was a problem hiding this comment.
I assumed that the format of the version (value) is always {number}.{number}.{number}
There was a problem hiding this comment.
we might need to add some error handling here
There was a problem hiding this comment.
I added this which filter non-digits chars out of the string.
I also added more test cases.
The main "risk" would be in the sort callback, and I added some guards as well see => https://github.com/nestjs/nest-cli/pull/2396/files#diff-088f2456e09a57e3cc418bccd4ab7a7d3968fb4fe3cd8e24a358f76ba6ad1473R177-R190
Eventually I can throw an error and catch it in the displayWarningMessage method to display a generic error message, but I think I covered all the cases with the last changes. What's you toughts @kamilmysliwiec ?
There was a problem hiding this comment.
Eventually I can throw an error and catch it in the displayWarningMessage method to display a generic error message, but I think I covered all the cases with the last changes. What's you toughts @kamilmysliwiec ?
We could add it just in case
There was a problem hiding this comment.
Done => d29182f
|
note that this rule doesn't apply to all packages under thus we need a whitelist of those that will be checked against. I think you can start with this:
I'm not sure about the others. I guess Kamil should tell us that |
More details here: nestjs#1929
GreGosPhaTos
force-pushed
the
feature/issues-1929
branch
from
3689e98
to
32b9380
Compare
|
@micalevisk added the white list directly in the method https://github.com/nestjs/nest-cli/pull/2396/files#diff-088f2456e09a57e3cc418bccd4ab7a7d3968fb4fe3cd8e24a358f76ba6ad1473R142-R151 |
actions/info.action.ts
Outdated
| @@ -21,6 +21,11 @@ interface PackageJsonDependencies { | |||
| interface NestDependency { | |||
| name: string; | |||
| value: string; | |||
| fullName: string; | |||
There was a problem hiding this comment.
can we rename fullName to packageName?
also, do npm run format
Added few changes after the review More details here: nestjs#1929
Added few changes after the review More details here: nestjs#1929
|
Just wondering is this PR needs some changes ? Otherwise should it be merged soon ? |
|
Looks fine to me so far I guess it will be merged after Kamil get some time to review this too, don't worry. |
|
@GreGosPhaTos btw could you please open a PR at https://github.com/nestjs/docs.nestjs.com to document this new behavior of the |
@micalevisk Yes ok => here it is nestjs/docs.nestjs.com#2923 |
|
LGTM |
More details here: #1929
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?
There is no warnin message if deveopers use different version of the nest-js package in a projet
Issue Number: #1929
What is the new behavior?
Developer now gets a warning message folowing the suggestion => #1929
Does this PR introduce a breaking change?
Other information