-
-
Notifications
You must be signed in to change notification settings - Fork 206
fix: handle null package.json name when parsing and matching #1877
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: handle null package.json name when parsing and matching #1877
Conversation
lib/helpers/utils.js
Outdated
const match = (typeof name === "object" ? name.name || "" : name || "").match( | ||
nameRegExp, | ||
); | ||
const name = (typeof name === "object" && name) ? name.name || "" : name || ""; |
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.
chatgpt is suggesting like this. wdyt?
// ensure we end up with a string, whether name is an object or a primitive
const processedName = typeof name === 'object'
? name.name ?? ''
: name ?? '';
// use exec if you only need the first match (avoids oddities with global regexes)
const match = nameRegExp.exec(processedName);
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 would have the same issue of not handling nulls properly. typeof name === 'object'
is true
, so when it lands in the first branch of the ternary, name.name
errors like it does now. Alternatively, if the project doesn't need to support older runtimes and/or TS downlevels enough, we can do just name?.name
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.
ah, looks like we cannot reuse the variable name
here.
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.
don't know why I am vibe coding. how about this one?
// normalize to a string, safely handling null/undefined and objects
const safeName = String(name?.name ?? name ?? '');
// if you only need the first match, exec is a bit more predictable than match
const match = nameRegExp.exec(safeName);
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.
Done. I skipped the String()
conversion because the chain should handle nullish values and fall back to ''
if needed
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.
Thank you!
Is there a unit test or repo test we could add? |
@rlmestre any thoughts about unit tests and the proposed shorter fix? |
e1e8457
to
bccdf53
Compare
Added tests and simplified the code |
@rlmestre could you kindly rebase? Apologies, we had some major refactoring in other PRs. |
Signed-off-by: Rafael Mestre <277805+rlmestre@users.noreply.github.com>
Signed-off-by: Rafael Mestre <277805+rlmestre@users.noreply.github.com>
Signed-off-by: Rafael Mestre <277805+rlmestre@users.noreply.github.com>
bccdf53
to
4b2d41b
Compare
Closes #1876