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

fix: remove legacy assert #46068

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: remove legacy assert #46068

wants to merge 2 commits into from

Conversation

unional
Copy link

@unional unional commented Jan 3, 2023

fixes #46050

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 3, 2023
as now they always have `*`
@JakobJingleheimer
Copy link
Member

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
Copy link
Author

unional commented Jan 3, 2023

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
Copy link
Author

unional commented Jan 3, 2023

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
Copy link
Author

unional commented Jan 3, 2023

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

@unional
Copy link
Author

unional commented Jan 3, 2023

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
Copy link
Contributor

aduh95 commented Apr 13, 2023

@nodejs/loaders we should review this.

@GeoffreyBooth
Copy link
Member

Cc @guybedford

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PATTERN_KEY_COMPARE algorithm should be updated
5 participants