2
\$\begingroup\$

The idea of this code is to poll the server every 15 seconds to check for and update the notifications indicator. However, I'm not convinced I've done it right, as sometimes it seems to take an unusual amount of time to propagate across tabs and some users have reported that notifications "get stuck" and don't update at all. So here's the code, I think everything's right, but maybe I missed something.

$(function() {
    var LSping = "notify_lastping",
        LSresult = "notify_result",
        poll, interval = 15*1000,
        notif_icon = $("#notifs>a"),
        notif_body = $("#notifications");

    if( notif_icon.length < 1) return;

    var notbox = null,
        lastping = -1,
        running = false;

    function startPoll() {
        var delay = interval;
        if( lastping == -1) {
            delay = (+localStorage[LSping]+interval || 0) - Date.now();
            if( delay < 100) delay = 100;
        }
        lastping = localStorage[LSping] || 0;
        if( poll) clearTimeout(poll);
        poll = setTimeout(getNotifications,delay);
    }
    function getNotifications() {
        // storage event should reset the poll and update notices for us,
        // but just to be sure...
        if( lastping != (localStorage[LSping] || 0)) {
            updateNotifications(
                JSON.parse(localStorage[LSresult] || '{"html":"","count":0}'),
                true
            );
            return startPoll();
        }

        // inform other tabs that I'm on this
        localStorage[LSping] = Date.now();

        if( !running) {
            running = true;
            ajax("/notices").success(function(r) {
                running = false;
                localStorage[LSresult] = JSON.stringify(r);
                updateNotifications(r);
            }).failure(function(r) {
                running = false;
                return true; // use default error handler for AJAX callback
            });
        }

        startPoll();
    }
    function updateNotifications(r,fromStorage) {
        if( !fromStorage) {
            var oldids = notif_body.find("[data-id]").map(function() {return this.getAttribute("data-id");}).get();
        }
        notif_icon.attr("data-count",r.count);
        notif_body.html(r.html);
        document.title = (r.count ? "["+r.count+"] " : "")+document.title.replace(/^\[\d+\]\s*/,"");

        if( !fromStorage) {
            var newids = notif_body.find("[data-id]").map(function() {return this.getAttribute("data-id");}).get();
            var diff = $(newids).not(oldids).get(); // damn jQuery can do some weird stuff XD
            if( diff.length > 0) {
                // play notification sound if selected by user settings
            }
        }
    }

    getNotifications();

    $(window).on("storage",function(e) {
        e = e.originalEvent;
        if( e.key == LSping) {
            startPoll();
        }
        if( e.key == LSresult) {
            updateNotifications(JSON.parse(localStorage[LSresult]),true);
        }
    });
});

As far as I can tell there's no reason for the "loop" to stop. getNotifications() calls startPoll() regardless of the AJAX result. But like I said, maybe I missed something.

\$\endgroup\$
3
\$\begingroup\$

I would try to also add a complete callback function to set running to false. It handles ( ("success", "notmodified", "nocontent", "error", "timeout", "abort", or "parsererror")) Those should be handled by error and success but something is going on, so give it a shot?

Other than, your code is quite solid, I ran JSHint over it, and it got very few hits.

  1. You never actually use notbox
  2. You probably want to declare oldids before that if loop

Other than that

  • The naming is definitely a mixed bag, I would stick everywhere with lowerCamelCase ( lastping -> lastPing, notif_body -> notificationsBody ( or consider notificationsElementor perhaps just notifications )

  • You are using one liner ifs without curly braces.

Final thought, are you at any point rebuilding the DOM? If you rebuild the DOM (specifically any of the elements captured by notif_ico and notif_body), then your code stops working.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ notbox is used in the original source, an extra bit of presentation for the .error handler. I trimmed out some of the extra bits before posting here, and must have missed it once. I see what you mean about variable names, they are a bit all over the place. And lastly, while there are plenty of DOM modifications, the notif_icon and notif_body elements don't change. Even so, I should probably add checks or re-evaluate the selectors... Thanks for your insights. \$\endgroup\$ – Niet the Dark Absol Sep 15 '16 at 15:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.