-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: require.main.filename check in DefaultCredentialManager
#2297
Conversation
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2297 +/- ##
=======================================
Coverage 91.06% 91.06%
=======================================
Files 628 628
Lines 17867 17867
Branches 3735 3805 +70
=======================================
Hits 16271 16271
Misses 1595 1595
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Trae Yelovich <[email protected]>
Quality Gate passedIssues Measures |
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 @traeok!
if (require.main.filename != null) { | ||
requireOpts.paths = [require.main.filename, ...require.resolve.paths("@zowe/secrets-for-zowe-sdk")]; | ||
} | ||
requireOpts.paths = [require.main?.filename, ...require.resolve.paths("@zowe/secrets-for-zowe-sdk")].filter(Boolean); |
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.
Technically this changes the original behavior so that we always pass in a list of paths to require.resolve
, but I don't think this change should be impactful 😋
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.
This is a good point - I don't want to make any assumptions, and its possible that the default module resolution is special for certain projects. So, I've updated this to add optional chaining to the if
check so that we only apply the paths if a filename is provided in require.main
😋 that way we keep the same behavior as before, but it still resolves the TypeError encountered in the attached issue.
Signed-off-by: Trae Yelovich <[email protected]>
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
In some environments (namely ESM and the Node.js REPL), the
main
property is not always available within therequire
object, so theif
check forrequire.main.filename != null
results in a TypeError.How to Test
This one is a bit difficult to test, but this should work as an example:
Make an empty folder and set up a basic NPM project using
npm init
Build a TGZ for
@zowe/imperative
usingnpm install && npm run build && npm pack
in thepackages/imperative
folder. Install the generated tarball in this new NPM project alongside@zowe/secrets-for-zowe-sdk
as normal dependencies:npm install zowe-imperative-8.1.0.tgz @zowe/secrets-for-zowe-sdk
Open the Node.js REPL within this folder:
node -i
Import
DefaultCredentialManager
within the REPL and create a new instance of it. Then attempt to initialize it. Initialization should be successful with no errors thrown:Review Checklist
I certify that I have: