-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Use read-pkg-up
over pkg-up
.
#680
Conversation
I can't seem to find why this is failing. Does anyone with more experience with |
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.
Hey @wtgtybhertgeghgtwtg! Thanks for the PR and sorry for the delay.
I made comments that should help you fix the tests :)
@@ -1,18 +1,14 @@ | |||
import fs from 'fs' |
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 you remove this import, now that it is not used?
try { | ||
const packageContent = JSON.parse(fs.readFileSync(filepath, 'utf8')) | ||
const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false}) | ||
const packageContent = JSON.parse(pkg.pkg) |
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.
The problem that makes the build fail is that getDependencies
is now returning something else. pkg.pkg
is now an object, and not a string that remains to be parsed. By attempting to parse it you are causing an error, which will be cached and make the function return null
.
This works better:
const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false})
if (!pkg || !pkg.pkg) {
return null
}
const packageContent = pkg.pkg
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, I don't know how I missed that.
Thanks for the changes :) LGTM, but do you know why the whole file is considered a having been changed in that last commit, instead of only the modified lines? That's pretty odd 🤔 |
I accidently saved it as CRLF instead of LF. That should be fixed now. |
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.
Awesome, the diff is much easier to read now 😅
LGTM, thanks for your work on this :)
I'll give some time to @benmosher and @ljharb to review, and I'll merge this in a few days if they dontt have the time to look at it.
Thank you for helping me with this Pull Request. |
thanks @wtgtybhertgeghgtwtg! |
Thank you for merging. |
Closes #675.