-
Notifications
You must be signed in to change notification settings - Fork 88
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
Use Fewer Deprecated Functions in Master #2217
Conversation
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
packages/zosfiles/__tests__/__unit__/methods/utilities/Utilities.unit.test.ts
Fixed
Show fixed
Hide fixed
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2217 +/- ##
==========================================
- Coverage 91.16% 91.15% -0.01%
==========================================
Files 638 638
Lines 19134 19103 -31
Branches 4041 4041
==========================================
- Hits 17444 17414 -30
+ Misses 1689 1688 -1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Please wait to merge - system tests are in progress. |
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
@@ -2,6 +2,10 @@ | |||
|
|||
All notable changes to the Zowe CLI test utils package will be documented in this file. | |||
|
|||
## Recent Changes | |||
|
|||
- BugFix: Refactored code to reduce the use of deprecated functions to prepare for Node 22 support. [#2191](https://github.com/zowe/zowe-cli/issues/2191) |
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.
Can we explain why we need Node 22 support?
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 don't quite know how to explain it in the changelog. The CLI supports running on all Node LTS versions that are actively supported by the NodeJS project maintainers, and Node 22 will reach that point in October. That commitment is documented in the Zowe CLI system requirements. This PR sets the ground work to honor that commitment.
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.
How about: " ... upcoming NodeJS 22 support"
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.
Sounds good. Changelogs updated. Thanks!
System Tests are complete and pass. |
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
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.
Looks good, thanks @awharn!
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.
LGTM! 😋
All of my comments are mostly minor things that can be done at a later time, or be forgotten since they should not impact performance or behavior 😋
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.
Is there a way we could avoid having to maintain a separate tsconfig.json file in every package?
I'm thinking of techniques similar to how jest config can be combined.
Not really requesting changes, though! 😋
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.
We made the same change in Next, but no, I don't think there is anything we can do. ESLint expects a TSConfig everywhere it scans, and you need different TSConfigs between tests and source. We could try to have them all extend a common TSConfig, but that would need to be a separate effort in V3. I don't expect we will change any of these TSConfigs through the rest of V2.
@@ -274,7 +274,7 @@ export class TextUtils { | |||
* @returns {string} - a formatted string with the variables inserted | |||
*/ | |||
public static formatMessage(message: string, ...values: any[]): string { | |||
if (!isNullOrUndefined(values)) { | |||
if (!(values == null)) { |
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.
There are multiple occasions (in this file and maybe others) where we do !(a == null)
instead of a != null
Curious if we could replace them with the shorthand version of != null
😋
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.
That is what the hundred plus issues flagged are. I just did this as shorthand since when I did that on V3, I made several mistakes when trying to do exactly that change that ended up breaking people and causing issues. This will all go away in V3, I just wanted to minimize the amount of time spent and mistakes made in V2.
packages/imperative/src/security/__tests__/DefaultCredentialManager.unit.test.ts
Outdated
Show resolved
Hide resolved
@@ -200,12 +200,12 @@ export abstract class AbstractProfileManager<T extends IProfileTypeConfiguration | |||
if (parms.loadCounter != null) { | |||
this.mLoadCounter = parms.loadCounter; | |||
} | |||
this.mLogger = isNullOrUndefined(parms.logger) ? this.mLogger : parms.logger; | |||
this.mLogger = parms.logger == null ? this.mLogger : parms.logger; |
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.
It is possible that this comment may apply to other files...
Curious if we should take advantage of other operators like ??
😋
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 was just doing a find and replace. I was not evaluating any of the other logic. I would think if we want to change something like this, it might make more sense as another issue in order to reduce scope creep.
this.mProfileType = parms.type; | ||
this.mProfileRootDirectory = parms.profileRootDirectory; | ||
this.mProfileTypeConfigurations = parms.typeConfigurations; | ||
this.mProductDisplayName = parms.productDisplayName; | ||
if (isNullOrUndefined(this.profileTypeConfigurations) || this.profileTypeConfigurations.length === 0) { | ||
if (this.profileTypeConfigurations == null || this.profileTypeConfigurations.length === 0) { |
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.
Similar to the previous comment...
what about using ?.
😋
There are edge cases where ?.
may not be desired since it can technically change the behavior. For example, if (a != null && !a.booleanValue)
is not the same as if (!a?.booleanValue)
since the condition will evaluate to true if A
is undefined 😋
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.
Same as previous comment.
@@ -21,6 +21,7 @@ import * as yargs from "yargs"; | |||
import { ImperativeError } from "../../error/src/ImperativeError"; | |||
|
|||
describe("Imperative", () => { | |||
// eslint-disable-next-line deprecation/deprecation | |||
const mainModule = process.mainModule; |
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.
Curious if there is a way to configure the deprecation/deprecation eslint plug-in to ignore all uses of process.mainModule
😋
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.
There is not. I checked.
/** | ||
* Loaded z/OSMF profile if needed | ||
* @deprecated | ||
*/ | ||
protected mZosmfProfile: IProfile; | ||
|
||
/** | ||
* Loaded z/OSMF profile with meta information | ||
* @deprecated | ||
*/ | ||
protected mZosmfLoadedProfile: IProfileLoaded; |
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.
Are this newly deprecated properties?
I believe they still exist in the next branch 😋
Should we create an issue to remove them/replace them over there?
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 only deprecated them because the code around them didn't actually do anything, and the process of setting them used deprecated functions. I can undo this if we prefer.
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.
After in person discussion, we are planning to remove these in V3 since nothing sets the protected variables anymore.
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.
Looks pretty good, thanks @awharn!
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
Signed-off-by: Andrew W. Harn <andrew.harn@broadcom.com>
|
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.
LGTM, thanks @awharn!
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.
Thanks for the painstaking effort.
What It Does
Resolves #2191
Reduces the use of deprecated functions in preparation to support Node 22
How to Test
Install, build, run commands to verify new build does not trigger deprecation warnings on Node 22
Review Checklist
I certify that I have:
Additional Comments