[rush] Fix edge cases where Rush does not update the lockfile

[L-Qun]

Details

If the user moves a package from dependencies to devDependencies, and then runs rush update, Rush should update the lockfile correctly, but it does not. This issue occurs because it triggers case DependencyType.Dev, but Rush cannot find this package in importer.devDependencies. Consequently, Rush deletes it from the importerDependencies set.

Impacted documentation

None.

[iclanton]

Suggested change

// eslint-disable-next-line no-fallthrough

[iclanton]

Given that this explicit rule suppression is here, it seems like there was a reason for this behavior initially. Are you sure unit tests adequately cover all scenarios here?

[L-Qun]

I'm not sure. But I noticed that you added this rule suppression. Is there a specific case for that?

[L-Qun]

Or maybe I guess it was just an oversight.

[octogonz-dev]

@chengcyber and I reviewed this today. It is really a spec change, not a bug:

In the past, whether a package was a devDependency or regular dependency did not affect the node_modules folder. The distinction maybe was reflected in the lockfile, but the installation plan was equivalent.

But now, apparently rush install fails if PNPM finds that the lockfile is inconsistent with package.json with regards to devDependencies-vs-dependencies. We need to modify the spec, such that Rush's analysis now will treat devDependencies vs dependencies as a significant difference.

This PR technically fixes the behavior by inserting a break statement, however the change is a bit confusing, because the code comments and structure are unchanged. The /intent/ of the code appears to be the same, but in fact we have changed it.

Possible improvements:

1. Add some code comments better explaining the expected behavior
2. Possibly restructure the code to no longer rely on case statements that fallthrough
3. Ideally add some unit tests with examples of the edge cases

[iclanton]

This should probably be an error case.

[iclanton]

Making this an error case would likely mitigate whatever issue the existing code was trying to account for.

[octogonz-dev]

Making this an error case would likely mitigate whatever issue the existing code was trying to account for.

I'm not sure about that. If the inconsistency is in an indirect dependency, and PNPM tolerates it, then Rush probably should as well.