[rush] Fix for issue 1349 rush install fails for preferred version

[apostolisms]

This is a fix for issue #1349

[halfnibble]

For the else case, do we need to grab the last element on split('/') and then grab the first element on split('_')?

[sachinjoseph]

Extract dependencyVersion.split('/').pop()! into a variable so that it need not be computed twice?

[iclanton]

I don't think this is what you want. From that issue, it looks like the two formats we need to handle are:

• /react-loadable/5.5.0/react@16.8.6
• /react-loadable/5.5.0_react@16.8.6

(both matching react@16.8.6)

The dependencyVersion.split('/').pop() statement will return react@16.8.6, but semver.valid('react@16.8.6') returns null, always falling back to the dependencyVersion.split('_') statement.

I think this is what we want:

Suggested change

	return semver.valid(dependencyVersion.split('/').pop()!) ?
	if (dependencyVersion.indexOf('_') !== -1) {
	// PNPM 3: the format looks like "/react-loadable/5.5.0_react@16.8.6"
	return dependencyVersion.split('_').pop()!;
	} else {
	// PNPM 2: the format looks like "/react-loadable/5.5.0/react@16.8.6"
	return dependencyVersion.split('/').pop()!;
	}

[apostolisms]

@iclanton in PNP 2.x it seems like its coming in as:
/react-loadable/5.5.0 and
/react-loadable/5.5.0/react@16.8.6
and similarly for react-dom. So semver.valid('5.5.0') or semver.valid('16.8.6') passes

In pnpm 3.x it is consistently:
/react-loadable/5.5.0_react@16.8.6
/react-dom/16.8.6_react@16.8.6
and dependencyVersion.split('_')[0] will validate semver.valid('5.5.0')

If we use dependencyVersion.split('_').pop()!; it will fail in pnpm 3.x because it will try to validate react@16.8.6

I think we need to return dependencyVersion.split('_')[0] for pnpm 3.x to pass regardless if we check using dependencyVersion.indexOf('_') !== -1 or semver.valid

[iclanton]

I misunderstood what the input to this function was. I thought it was the full path, not just a portion of the version string.

None of the inputs to this function when it runs on WBT contain any slashes. That makes me wonder if the logic introduced in #615 is necessary for anything other than PNPM v1. @halfnibble is probably right that .split('_') should be called after the slashes are split off.

[iclanton]

I misunderstood what the input to this function was. I thought it was the full path, not just a portion of the version string.

None of the inputs to this function when it runs on WBT contain any slashes. That makes me wonder if the logic introduced in #615 is necessary for anything other than PNPM v1. @halfnibble is probably right that .split('_') should be called after the slashes are split off.

[apostolisms]

@iclanton it seems like it. There are no slashes with pnpm 2.x. If it was introduced for pnpm 1.x support maybe we should keep it for backwards compatibility. Also I want to update per @sachinjoseph recommendation before merging.

[octogonz]

The original code was being a bit lazy with dependencyVersion.split('/').pop()!. (I like to say "If a function relies on split(), it probably has at least one bug" -- but of course that code predates your PR heheh.)

Now that we're expanding the logic, it might be time to beef up the rigor a bit, since:

• The input format is no longer trivially simple
• This input comes from an unvalidated external file, and that file has already betrayed us once

Anyway the next step up is a RegExp, something like this (pseudocode):

class TheClass {
  // Example: /react-loadable/5.5.0
  private static _pnpm2VersionRegExp: RegExp = /\/([^/_@]+)$/;

  // Example: 5.5.0_react@16.8.6
  private static _pnpm3VersionRegExp = /^([^/_@]+)_/;

   private _getValidDependencyVersion(dependencyVersion: string): string {
     let match: RegExpExecArray | null = MyClass._pnpm2VersionRegExp.exec(dependencyVersion);
     if (match) {
       return match[1];
     }
     match = MyClass._pnpm3VersionRegExp.exec(dependencyVersion);
     if (match) {
       return match[1];
     }
     throw new Error(`Invalid version format: "${dependencyVersion}"`)
   }
}

[octogonz]

The semver.valid() thing also seems a bit lax. It's basically saying "If it doesn't look like a version, meh, let's pretend we didn't see it." Ideally we'd want to have a clear idea about what exactly we're ignoring, but I don't want to hold up your PR, since it's an important fix. :-)

[apostolisms]

@octogonz I think we should merge this in since it is a blocking issue and open a separate PR for optimizing. I don't particularly like my fix either since even dependencyVersion.split('/').pop()! seems to be redundant. With pnpm 2.x it looks like dependency version never comes in as /react-loadable/5.5.0 I think the slashes get stripped out elsewhere?
I don't want to depart too much from the original implementation though before I thoroughly test it with 2.x and 3.x and verify that the cases we need to handle are actually just /react-loadable/5.5.0 and /react-loadable/5.5.0_react@16.8.6 ( / and _)

[octogonz]

Sounds good 👍