fix: remove lib/utils/read-package-name.js #4757
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wraithgar
changed the title
wraithgar
force-pushed
the
gar/read-package-name
branch
from
c7b8b71
to
6f2b996
Compare
|
no statistically significant performance changes detected timing results
|
lukekarrys
suggested changes
Apr 16, 2022
lib/commands/diff.js
Outdated
| @@ -7,6 +7,7 @@ const pacote = require('pacote') | |||
| const pickManifest = require('npm-pick-manifest') | |||
| const log = require('../utils/log-shim') | |||
| const readPackageName = require('../utils/read-package-name.js') | |||
There was a problem hiding this comment.
It seems like this is used a few other places in this file. Can it be removed from those too?
There was a problem hiding this comment.
I thought i got them all. It's very concerning that tests still pass here.
There was a problem hiding this comment.
also i think the actual util file didn't get deleted, only the test
There was a problem hiding this comment.
This is why we review. Removed.
There was a problem hiding this comment.
at least that explains why the tests passed previously 😅
This code wasn't doing anything special, just dereferencing `name` from a packument. There is no need for this to exist. Most of the tests were able to handle having this go away, except for `npm owner` which had to have its tests rewritten to be real, which of course surfaced bugs along the way of behavior that was incorrectly being tested. `npm owner` needs some love to clean up its UX, it throws or returns inconsistently. I did fix it so that if there is no package.json in cwd it errored as expected instead of throwing `ENOENT` which is what it did before.
wraithgar
force-pushed
the
gar/read-package-name
branch
from
6f2b996
to
a2b0c94
Compare
lukekarrys
approved these changes
Apr 16, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code wasn't doing anything special, just dereferencing
namefroma packument. There is no need for this to exist.
Most of the tests were able to handle having this go away, except for
npm ownerwhich had to have its tests rewritten to be real, which ofcourse surfaced bugs along the way of behavior that was incorrectly
being tested.
npm ownerneeds some love to clean up its UX, it throwsor returns inconsistently. I did fix it so that if there is no
package.json in cwd it errored as expected instead of throwing
ENOENTwhich is what it did before.