Skip to content
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(arborist): shrinkwrap throws when trying to read a folder without permissions #4258

Merged
merged 6 commits into from
Jan 26, 2022

Conversation

Linkgoron
Copy link
Contributor

This PR fixes a minor regression introduced here where Shrinkwrap.load throws an error instead of returning correctly when arborist doesn't have access to the folder it's trying to read.

The issue is caused by trying to log const rel = relpath(this.path, this.filename) when this.filename is null. this.filename is set after this[_maybeRead]() executes, but when there are permission issues this[_maybeRead]() throws, and this.filename stays null, and relpath (and consequentially, load) throws The "to" argument must be of type string error.

I've changed the code to only execute relpath if this.filename is set correctly.

…issions

Fix an issue where shrinkwrap throws an error when trying to read
a folder that it doesn't have permissions to, instead of returning
a correct object with an error
@Linkgoron Linkgoron requested a review from a team as a code owner January 18, 2022 23:03
@Linkgoron

This comment has been minimized.

@fritzy fritzy added the ws:arborist Related to the arborist workspace label Jan 19, 2022
@Linkgoron Linkgoron force-pushed the arborist-eaccess-error branch 2 times, most recently from 0282ef0 to 1910c8f Compare January 19, 2022 16:19
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Linkgoron 😄 I believe it's going to be a simpler and easier to maintain in the long term test if we just skip windows for now

workspaces/arborist/test/shrinkwrap.js Outdated Show resolved Hide resolved
workspaces/arborist/test/shrinkwrap.js Outdated Show resolved Hide resolved
workspaces/arborist/test/shrinkwrap.js Outdated Show resolved Hide resolved
@ruyadorno ruyadorno added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release labels Jan 20, 2022
@wraithgar wraithgar changed the base branch from latest to release-next January 26, 2022 20:43
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@ruyadorno ruyadorno merged commit 8c3b143 into npm:release-next Jan 26, 2022
@wraithgar wraithgar mentioned this pull request Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes ws:arborist Related to the arborist workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants