fix: remove legacy assert#46068
Conversation
|
Review requested:
|
as now they always have `*`
|
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. |
|
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? |
|
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
} |
|
Or, I can at least add some comments there to indicate the code supports a legacy/deprecated algorithm |
|
btw, I have a question about the The code in Node.js and in 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? |
|
@nodejs/loaders we should review this. |
|
Cc @guybedford |
fixes #46050