Significant Events Endpoint for Article as a Living Document experiment

[tonisevener]

https://phabricator.wikimedia.org/T241253

[joewalsh]

Looking great overall! Just found one minor issue

[joewalsh]

This titleDict is a copy of the value inside domainDict so updating it doesn't update the value inside of domainDict. This could be fixed by moving domainDict[keyTitle] = titleDict; outside of the if/else block so that the updated value is set in the domainDict in either case. I'm not sure if there's a way to get reference semantics here so you could update directly

[joewalsh]

Same comment as above but for the domainDict, should be fixed by moving significantChangesCache[req.params.domain] = domainDict; below out of the if/else block

[joewalsh]

I noticed cachedOutput is always empty, even on subsequent requests for the same title. Seems to be fixed by making the changes suggested below

[joewalsh]

Given the comments above, and that || in JS works kind of like ?? in Swift, this could be updated to:

function setSignificantChangesCache(req, title, item) {
    const keyTitle = keyTitleForCache(req, title);
    var domainDict = significantChangesCache[req.params.domain] || {};
    var titleDict = domainDict[keyTitle] || {};
    titleDict[item.revid] = item;
    titleDict.maxedOut = calculateCacheForTitleIsMaxedOut(titleDict);
    domainDict[keyTitle] = titleDict;
    significantChangesCache[req.params.domain] = domainDict;
}

[joewalsh]

If you had:

    var talkPageTitle = talkPageTitle(req);

the function is already shaded by the variable name. Could be fixed by renaming the function to getTalkPageTitle or using a different variable name