Reject packages that do not have path.

[krzysztof-pawlik-gat]

This could happen for package.json files for which npm correctly
shows an error, but yarn works with them just fine.

As it is not a correct npm case it's not trivial to add a test for this case (as it will be rejected by npm), this will become a moot issue when proper yarn source is introduced. This happened in a case where there was a dependency conflict, one of the libraries deep down required specific version of lodash and at top level we require the latest one. The error was happening for the version deep down, npm produced following list in JSON:

        "lodash": {
          "requiredBy": "4.6.1",
          "missing": true,
          "optional": false
        }

Note the lack of version and path, but it has missing set.

[jonabc]

👋 @krzysztof-pawlik-gat thanks for opening this up!

I'm not terribly familiar with yarn's requirements or what is considered an error case. Is a missing path/dependency not considered an error for yarn?

I'm hesitant about this approach because it's either hiding an error that could be reported to the user or it's hiding a second version of a dependency used in the product. For full dependency reporting both the top level version of lodash as well as the missing 4.6.1 should be listed in a bill of materials.

this will become a moot issue when proper yarn source is introduced

I took a quick look at a yarn source a few months ago and couldn't find a way to easily enumerate all the information that licensed needs. If you're familiar with the yarn CLI and how to pull package metadata and license information I would gladly accept a PR for this.

[krzysztof-pawlik-gat]

This is not exactly yarn's fault -- it does show a warning for this:

[1/4] 🔍  Resolving packages...
warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"
warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"
[2/4] 🚚  Fetching packages...

But it continues and installs the latest version, npm sees the package as missing (note that it's even marked as such). I've changed the code to skip missing packages instead of those without path, I hope this makes more sense and looks better.

[jonabc]

@krzysztof-pawlik-gat sorry for the back and forth on this, I'm still concerned about a few things.

warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"
warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"

What is the nature of this warning? Is it an error/warning that the yarn community at large ignores or do you only want to ignore it for your use case? Are [email protected] and [email protected] ever installed and included in production/external deployments of the project?

My other concern is that the change is adding yarn-specific functionality that will end up affecting users that aren't using yarn. Would it make sense to ignore missing packages only when a yarnfile.lock is present, as a differentiator between yarn and npm users?

[krzysztof-pawlik-gat]

@krzysztof-pawlik-gat sorry for the back and forth on this, I'm still concerned about a few things.

No problem at all, that's the purpose of reviews :)

warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"
warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"

What is the nature of this warning? Is it an error/warning that the yarn community at large ignores or do you only want to ignore it for your use case?

I've seen such warnings a few times, while it's good to not have them, it looks like it's not extremely important, as ...

Are [email protected] and [email protected] ever installed and included in production/external deployments of the project?

No, those old (and vulnerable) versions are not installed, only the latest one (4.17.15) ends up in node_modules:

$ find node_modules/ -name lodash
node_modules/lodash

From my testing it looks like the missing: true will be when packages are installed by yarn, but npm is used to list them. npm will happily install multiple versions of the same package:

$ find node_modules/ -name lodash
node_modules/graphql-toolkit/node_modules/lodash
node_modules/lodash
node_modules/gat/node_modules/lodash

My other concern is that the change is adding yarn-specific functionality that will end up affecting users that aren't using yarn. Would it make sense to ignore missing packages only when a yarnfile.lock is present, as a differentiator between yarn and npm users?

Good point, I've added a check for yarn.lock before discarding the missing dependency.

(fixed some EN issues)

[jonabc]

Sorry for the delay on this. I really appreciate you helping me understand the scope of the issue!

It's weird to be that yarn would have a message for a non-error case, 🤷, but this should be safe and its great that it's targeted.

Would you also mind adding a test to verify the new behavior when a yarn lockfile is present?

Let me know if you have any questions or concerns!

[krzysztof-pawlik-gat]

Added tests for this case.

[jonabc]

a couple comments on the added test cases, let me know if you have any questions or if anything I'm asking for doesn't make sense 🙏

[krzysztof-pawlik-gat]

@jonabc Travis shows a failure for Haskel/cabal, this was not touched at all in this change, can you trigger the build again? (I don't want to push empty commits).

[krzysztof-pawlik-gat]

Comments addressed, tests passing, I think this is ready for another round of review :)

[jonabc]

Thanks for the updates!

I'm not sure that the new tests really need a new fixtures folder, but either way I'm only really concerned about unexpected test behavior if the yarn lockfile sticks around the local fixtures folder

[krzysztof-pawlik-gat]

Reworked tests to use a temporary directory (like bundler tests do).

[jonabc]

thanks again for the changes, everything looks great.