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

"//" keys should be ignored in "resolution" section of package.json #4774

Closed
Peeja opened this issue Oct 24, 2017 · 2 comments
Closed

"//" keys should be ignored in "resolution" section of package.json #4774

Peeja opened this issue Oct 24, 2017 · 2 comments

Comments

@Peeja
Copy link

Peeja commented Oct 24, 2017

What is the current behavior?

"//" keys (comments) in the "resolution" section are not ignored.

Steps to reproduce:

  1. Add a key to the "resolution" section of a package.json like:

    {
      "resolution": {
        "//": "This should be a comment."
      }
    }
    
  2. Run yarn install.

  3. See warning:

    warning Resolution field "//" does not end with a valid package name and will be ignored
    

What is the expected behavior?

The key should be ignored. "//" keys are a standard way of commenting in package.json files in lieu of actual comments being available in JSON. (The "resolution" section in particular is a place where comments are necessary, so you can communicate why you're doing heavy-handed dependency tweaking.)

Please mention your node.js, yarn and operating system version.

  • Node v8.7.0
  • Yarn v1.2.1
  • macOS 10.12.6 (16G29)
@rally25rs
Copy link
Contributor

rally25rs commented Oct 24, 2017


~~~It seems to work for me in Yarn 1.2.1~~~

Edit: the section name should be "resolution**s**" not "resolution" :)

Now I see the same warning.

Ah I guess it is noted in #3829 that:

>  I'd also avoid cleaning up all // keys and limit it to dependencies, devDependencies etc. 

so it looks like we just need to add `resolutions` to that list. Should be super easy. I'll get a PR for this tomorrow.

@rally25rs rally25rs self-assigned this Oct 24, 2017
rally25rs added a commit to rally25rs/yarn that referenced this issue Oct 25, 2017
**Summary**

Previously package.json comments were being ignored for "dependencies",
"devDependencies", "optionalDependencies".

This change adds "resolutions" to the sections that will ignore
comments.

**Test Plan**

Added unit test to make sure warning is not printed for a comment in a
resoluton.

Fixes yarnpkg#4774
@BYK BYK closed this as completed in #4779 Oct 26, 2017
BYK pushed a commit that referenced this issue Oct 26, 2017
…4779)

Fixes #4774

**Summary**

Previously package.json comments were being ignored for "dependencies",
"devDependencies", "optionalDependencies".

This change adds "resolutions" to the sections that will ignore
comments.

**Test Plan**

Added unit test to make sure warning is not printed for a comment in a
resolution.
@Peeja
Copy link
Author

Peeja commented Oct 26, 2017

@rally25rs Thanks!

joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
…arnpkg#4779)

Fixes yarnpkg#4774

**Summary**

Previously package.json comments were being ignored for "dependencies",
"devDependencies", "optionalDependencies".

This change adds "resolutions" to the sections that will ignore
comments.

**Test Plan**

Added unit test to make sure warning is not printed for a comment in a
resolution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants