-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
refactor: general refactor (explicit types, generics, components) #1312
Conversation
Pull Request Test Coverage Report for Build 1232
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1237
💛 - Coveralls |
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.
Really great step forward!
That is what I call one productive "break"
What about https://github.com/nestjs/nest/pull/1307/files#diff-bda512087c3a50709e8088dbaecfe8c6L39
showDeprecatedWarnings
overrideModuleMetadata
? Shouldn't these also be removed?
return this.override(typeOrToken, false); | ||
} | ||
|
||
/** @deprecated */ | ||
public overrideComponent(typeOrToken): OverrideBy { | ||
public overrideComponent<T = any>(typeOrToken: T): OverrideBy { |
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.
Should this still exist? Shouldn't we just let it fail in case someone actually uses components
?
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.
I think that all changes that have been made so far, don't have to be considered as "breaking changes" which means that we wouldn't have to bump major version. That thing on the other hand (overrideComponent
) should be probably removed in another PR :D
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.
Oh I see.. Then I am fine with this PR :)
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information