make acceptedLanguages order the header by q values descending #262
Conversation
| return tokens.filter(t => t !== "").map(t => t.split(";")[0]); | ||
| const tokens = acceptLanguageHeader.split(",").map(t => t.trim()); | ||
| const langsWithQ = []; | ||
| tokens.filter(t => t !== "").forEach((t, index) => { |
There was a problem hiding this comment.
Can you please move the filter to line 5 together with trimming?
| const langsWithQ = []; | ||
| tokens.filter(t => t !== "").forEach((t, index) => { | ||
| const langWithQ = t.split(";").map(u => u.trim()); | ||
| if (langWithQ[0].length > 0) { |
There was a problem hiding this comment.
String.prototype.split always returns at least one element, so no need for this check I think?
There was a problem hiding this comment.
yes, that's true -- removed
| const qVal = langWithQ[1].split("=").map(u => u.trim()); | ||
| if (qVal.length === 2 && qVal[0].toLowerCase() === "q") { | ||
| const qn = Number(qVal[1]); | ||
| q = !isNaN(qn) ? qn : q; |
There was a problem hiding this comment.
That means that any non-numerical q value will get replaced with 1.0, is that what we should do? Seems like bogus input should result in low priority, not high.
There was a problem hiding this comment.
Seems fair -- I've replaced it with 0.0. (I was using q=1 here because it's the default value in the specification)
|
I'm not really sure what you meant by the first comment -- I've moved the code for parsing a list entry (fr;q=0.5) to a separate function, is that what you meant? |
|
Any chance of picking this one up? Thanks, Mark |
|
@zbraniecki ping :) |
| tokens.forEach((t, index) => | ||
| langsWithQ.push(parseAcceptLanguageEntry(t, index))); | ||
| // order by q descending, keeping the header order for equal weights | ||
| langsWithQ.sort((a, b) => a.q === b.q ? a.index - b.index : b.q - a.q); |
There was a problem hiding this comment.
Non-blocking cleanup suggestion:
function parseAcceptLanguageEntry(entry) {
const langWithQ = entry.split(";").map(u => u.trim());
let q = 1.0;
if (langWithQ.length > 1) {
const qVal = langWithQ[1].split("=").map(u => u.trim());
if (qVal.length === 2 && qVal[0].toLowerCase() === "q") {
const qn = Number(qVal[1]);
q = isNaN(qn) ? 0.0 : qn;
}
}
return { lang: langWithQ[0], q };
}
const langsWithQ = Array.from(tokens.map(parseAcceptLanguageEntry).entries());
// order by q descending, keeping the header order for equal weights
langsWithQ.sort(([aidx, aval], [bidx, bval]) => aval.q === bval.q ? aidx - bidx : bval.q - aval.q);
return langsWithQ.map(([idx, val]) => val.lang);I hand-written it so it may not launch but I hope the intention is expressed in my snippet. I'm trying to save us from reallocating the array in the loop. Alternatively, you could also likely do const langsWithQ = Array(tokens.length); which I guess is simpler.
The other thing I wanted to improve in my snippet is to not carry around the index, since the langsWithQ preserves the order of tokens, so there's no value in storing it on the langWithQ struct.
@blushingpenguin - do you like the proposed changes or would you prefer to land as-is?
There was a problem hiding this comment.
Nicer your way I think -- I wasn't aware of Array.entries. I have updated the code and retested it.
…to the language entry descriptor as suggested by zbraniecki's code review
I'm not sure if anything actually sends this header with languages listed out of descending preference order, but according to the specification you can't rely on it.