-
Notifications
You must be signed in to change notification settings - Fork 35
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(api): lint all dependencies in package-lock #53
fix(api): lint all dependencies in package-lock #53
Conversation
One thing I want to mention is this now will not longer catch something like
However, this I believe is a different issue all together and am willing to open an issue in regards to this. |
Codecov Report
@@ Coverage Diff @@
## master #53 +/- ##
==========================================
- Coverage 97.88% 95.08% -2.81%
==========================================
Files 11 11
Lines 189 183 -6
Branches 29 29
==========================================
- Hits 185 174 -11
- Misses 4 8 +4
- Partials 0 1 +1
Continue to review full report at Codecov.
|
Will not override in the object if there is a matching dep and version and will lint all the dependencies of dependencies. Fixes #49
@JamesSingleton can you elaborate your last comment about why it would not catch that kind of structure and what exactly will be missed? |
for (const [depName, depMetadata] of Object.entries(npmDepsTree)) { | ||
const depMetadataShortend = { | ||
version: depMetadata.version, | ||
resolved: depMetadata.resolved ? depMetadata.resolved : depMetadata.version, | ||
resolved: depMetadata.resolved, |
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.
@JamesSingleton can you comment why was this changed?
IIRC this was supposed to solve cases where a resolved property may not be there but a version would, for cases like local file URLs or something like this.
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.
So this is due to the new URL(metadata.resolved)
failing if this is a version?
I'll track back the source of this and we'll find out.
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.
From #49 (comment)
The error points to this line
try {
packageResolvedURL = new URL(packageMetadata.resolved)
} catch (error) {
throw new PackageError(packageName, error)
}
Not sure how this is now just coming up now though. Looks like this will happen with any dependency that does not have a true URL for resolved since resolved is optional and doing new URL("1.1.1") is not a valid URL.
Sounds like you're saying this is a general bug unrelated to this change.
I tracked the source of using the version to a package-lock.json
structure where the version
field is used to specify the git URL, see here: a483115#diff-24ebf59c7b0eeb6040c5311942ef4a3eR16
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.
@JamesSingleton nevermind here since we already have this discussion going on #54
@JamesSingleton I see you've taken the approach of keeping the object structure but with a modification for it being a hash. Did you find this solution better than just changing it to an array structure? |
You've also referred to an issue with one of the tests in the last comment in the issue, here: #49 (comment) Is this PR solving that or is that still something we need to fix? |
@lirantal, I felt that it was a better solution in my opinion. |
@JamesSingleton in any specific way? |
I keep the name and version along with the hash. The hash was just to make it unique and not have to redo all the validators. |
@JamesSingleton alright. I'm not entirely against doing as you proposed either but can assume that in the long run we will probably move to an array structure. I'm ok to move forward with this |
@lirantal can we move forward with merging this? |
Thank you @lirantal for taking time to work through and merge the PR! Question for you, when can we expect a release? |
New versions released 🎉
|
thanks @JamesSingleton for your contribution on this 💜 |
a regression introduced via PR #53 due to flat package list now being all-inclusive of packages, some of which have no protocol or URL to resolve to (the nested deps) and so the validators failed.
a regression introduced via PR #53 due to flat package list now being all-inclusive of packages, some of which have no protocol or URL to resolve to (the nested deps) and so the validators failed.
Will not override in the object if there is a matching dep and version
and will lint all the dependencies of dependencies.
Fixes #49
Description
Types of changes
Related Issue
#49
Motivation and Context
It solved the issue where if multiple dependencies had the same subdependencies (name and version) only one was added to the object. This was an issue if the first one was corrupt but the last one was fine, it would not be caught.
How Has This Been Tested?
Screenshots (if appropriate):
Checklist: