fix: remove legacy assert

[unional]

fixes #46050

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules

[JakobJingleheimer]

Hiya! Thanks for the contribution!

I'm not sure this is complete though: from the title, it sounds like an actual algorithm would be updated, but this only updates the spec.

[unional]

The actual implementation is here:

function patternKeyCompare(a, b) {
  const aPatternIndex = StringPrototypeIndexOf(a, '*');
  const bPatternIndex = StringPrototypeIndexOf(b, '*');
  const baseLenA = aPatternIndex === -1 ? a.length : aPatternIndex + 1;
  const baseLenB = bPatternIndex === -1 ? b.length : bPatternIndex + 1;
  if (baseLenA > baseLenB) return -1;
  if (baseLenB > baseLenA) return 1;
  if (aPatternIndex === -1) return 1;
  if (bPatternIndex === -1) return -1;
  if (a.length > b.length) return -1;
  if (b.length > a.length) return 1;
  return 0;
}

It does not do any assertion.

I can simplify the logic to:

function patternKeyCompare(a, b) {
  const aPatternIndex = StringPrototypeIndexOf(a, '*');
  const bPatternIndex = StringPrototypeIndexOf(b, '*');
  const baseLenA = aPatternIndex + 1;
  const baseLenB = bPatternIndex + 1;
  if (baseLenA > baseLenB) return -1;
  if (baseLenB > baseLenA) return 1;
  if (a.length > b.length) return -1;
  if (b.length > a.length) return 1;
  return 0;
}

But that would be a breaking change as I don't know if there are code out there hits the non-asserted cases.

So should I change it?

[unional]

In pattern-key-compare, I'm implementing it with assertions:

export function patternKeyCompare(keyA: string, keyB: string) {
  const iA = keyA.indexOf('*')
  const iB = keyB.indexOf('*')
  assert(iA !== -1, `'${keyA}' does not contain '*'`)
  assert(iB !== -1, `'${keyB}' does not contain '*'`)
  assert(keyA.lastIndexOf('*') === iA, `'${keyA}' has more than one '*'`)
  assert(keyB.lastIndexOf('*') === iB, `'${keyB}' has more than one '*'`)
  const baseLengthA = iA + 1
  const baseLengthB = iB + 1
  if (baseLengthA > baseLengthB) return -1
  if (baseLengthA < baseLengthB) return 1
  return keyA.length > keyB.length ? -1 : keyA.length < keyB.length ? 1 : 0
}

[unional]

Or, I can at least add some comments there to indicate the code supports a legacy/deprecated algorithm

[unional]

btw, I have a question about the conditions in the algorithm.

The code in Node.js and in resolve.exports both uses Set<string>.

Which does not handle:

{
  "exports": {
    "node": {
      "import": "./a.js"
    },
    "import": {
      "node": "./b.js"
     }
  }
}

Is that intentional or is that a bug or limitation due to legacy implementation?

[aduh95]

@nodejs/loaders we should review this.

[GeoffreyBooth]

Cc @guybedford