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: add npm view to transparent commands #158

Closed
wants to merge 1 commit into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Aug 10, 2022

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

@aduh95 aduh95 requested a review from arcanis August 15, 2022 08:31
@ljharb
Copy link
Member

ljharb commented Aug 15, 2022

it kind of seems like it'd be a more usable default to assume everything is transparent, and explicitly exclude the opaque ones.

@arcanis
Copy link
Contributor

arcanis commented Aug 15, 2022

it kind of seems like it'd be a more usable default to assume everything is transparent, and explicitly exclude the opaque ones.

Allowing new commands isn't a major change, whereas forbidding some is.

Generally I'm not sure how I feel about this change. While on surface it makes some sense, in practice it bypasses any registry configuration configured for the project (which can lead to potential security problems if the information you get is obtained from the public one instead of the private one). To avoid that one should call yarn npm info instead, which leverages the Yarn configuration.

There's an argument that this won't matter in most cases, but there's also the opposite argument: the cases where this matter are more likely to make this easy mistake unless the system prevents them to do so.

@ljharb
Copy link
Member

ljharb commented Aug 15, 2022

Allowing new commands isn't a major change, whereas forbidding some is.

that is a very good point. another alternative is disallowing all commands unless they're explicitly marked as opaque or transparent - then it'd always be semver-minor to mark a new command.

@aduh95
Copy link
Contributor

aduh95 commented Aug 19, 2022

@styfle What do you think of #158 (comment)? Going back to my idea of providing an ENV variable to disable the usage error would let user/CLI tools that know what they're doing bypass the Corepack check while avoiding to introduce errors for the most common case. (So you would do something like COREPACK_IGNORE_PACKAGE_JSON=1 npm view … or COREPACK_IGNORE_PACKAGE_JSON=1 npm install -g …)

@arcanis
Copy link
Contributor

arcanis commented Aug 19, 2022

Fwiw I agree an environment variable would be reasonable.

Bikeshedding the name, I'd suggest COREPACK_ENABLE_STRICT_CHECK (or CLI_CHECK, maybe), which would default to 1; it would match other similar settings, avoid the double negation, and is less ambiguous than just referencing the package.json.

@styfle
Copy link
Member Author

styfle commented Aug 24, 2022

How about COREPACK_ENABLE_STRICT=1 to enable the error and COREPACK_ENABLE_STRICT=0 to disable it?

@aduh95
Copy link
Contributor

aduh95 commented Sep 1, 2022

@styfle do you still want this merged now that #167 has landed?

@styfle
Copy link
Member Author

styfle commented Sep 2, 2022

While on surface it makes some sense, in practice it bypasses any registry configuration configured for the project

Given this, I think we should close and rely on the new COREPACK_ENABLE_STRICT=0 env var.

I'm also curious if we can make -g or --global transparent by default but perhaps I'm overlooking something in those cases too.

@styfle styfle closed this Sep 2, 2022
@styfle styfle deleted the patch-1 branch September 2, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants