3
\$\begingroup\$

I'm working in a Tableau driven system, with large numbers of charts, I needed a way to reduce the strain of the initial load of our app. It's fairly simple, but wanted to ensure I accounted for every scenario, I don't generally utilize lazy loading.

Print isn't a priority.

jsfiddle: Demo (Tableau doesn't work within SO's snippets because of the iframes)

Relevant Javascript

const throttle = (fn, delay) => {
    let canCall = true;
    return (...args) => {
      if (canCall) {
        fn.apply(null, args);
        canCall = false;
        setTimeout(() => {
          canCall = true;
        }, delay);
      }
    };
  };

  const setAttributes = (el, options) =>
    Object.keys(options).forEach(attr => el.setAttribute(attr, options[attr]));

  const w = window;
  const d = document;
  const b = d.body;
  const x = w.innerWidth || e.clientWidth || b.clientWidth;
  const y = w.innerHeight || e.clientHeight || b.clientHeight;

  /* unecessary, but to have same object format, e.g. readability */
  const win = Object.create(null);
  win.y = y;

  const frame = d.querySelectorAll("iframe");

  /** may be over doing it here... test results */
  const preloadPath = () =>
    frame.forEach(item => {
      const source = item.getAttribute("data-src");
      const preloadLink = d.createElement("link");
      const head = d.getElementsByTagName("head")[0];

      setAttributes(preloadLink, {
        rel: "preload",
        as: "document",
        type: "text/html",
        crossorigin: "anonymous",
        href: source
      });

      head.insertBefore(preloadLink, head.firstChild);
    });

  const loadFrame = () =>
    frame.forEach(item => {
      /* only run if not src is present */
      if (!item.src) {
        const source = item.getAttribute("data-src"); // path
        const parent = item.parentElement; // wrapper
        const frameRect = parent.getBoundingClientRect(); // wrapper dimensions

        if (
          w.pageYOffset + win.y >= frameRect.y && // if frame top exceeds page bottom
          frameRect.y + frameRect.height >= w.pageYOffset // if frame bottom exceeds page top
        ) {
          item.src = source; // set path
          item.removeAttribute("data-src"); // remove placeholder attr
        }
      }
    });

  /** load on docReady */
  d.addEventListener("DOMContentLoaded", [preloadPath, loadFrame]);

  /** throttle on scroll */
  const t = throttle(loadFrame, 64);
  w.addEventListener("scroll", t);
\$\endgroup\$
  • \$\begingroup\$ What is the purpose of preloadPath? \$\endgroup\$ – guest271314 Dec 20 '18 at 17:30
  • \$\begingroup\$ I was attempting to leverage preloading although I don't believe it is of any benefit here, using javascript to inject into the DOM isn't early enough in the lifecycle of the page load... If it is beneficial, it's negligible. \$\endgroup\$ – darcher Jan 1 at 15:05
  • \$\begingroup\$ If the purpose is to use <link> element is used to preload a resource, then load event of <link> should be used before loadFrame() is called. Presently preloadPath does not have direct relevance to the <iframe> elements, though can be utilized if import is used; see Is there a way to know if a link/script is still pending or has it failed; How to querySelector an element in an html import from a document that imports it?. \$\endgroup\$ – guest271314 Jan 1 at 15:55
  • \$\begingroup\$ That's actually a great point. I hadn't thought of that. Thanks. \$\endgroup\$ – darcher Jan 1 at 16:28
  • 1
    \$\begingroup\$ I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Jan 2 at 5:12
1
\$\begingroup\$

General Feedback

The code in the jsFiddle Demo appears to function well (although it isn't the same as the code above- see the next section for an explanation). There is good usage of const for values that are not re-assigned and functional programming with arrow functions.

Flaw with event listener setup

It appears that the fiddle has different code than appears here, but nonetheless, the code above contains the following line:

d.addEventListener("DOMContentLoaded", [preloadPath, loadFrame]);

The second argument appears to be an array. I haven't seen an array there used before and it doesn't appear to work. The Parameters section of the MDN documentation for addEventListener reads:

listener
The object which receives a notification (an object that implements the Event interface) when an event of the specified type occurs. This must be an object implementing the EventListener interface, or a JavaScript function. See The event listener callback for details on the callback itself.1

If you did need to have both functions run when that event occurs, you would either need to have a single callback function that calls both or call addEventListener once for each function. It appears that in the fiddle preloadPath has been removed...

Suggestions

Variable naming

The variable name frame sounds singular, yet it returns a NodeList which would typically contain multiple DOM elements.

const frame = d.querySelectorAll("iframe");

Thus a more appropriate name would be frames. That way when statements like frames.forEach() is read, it implies that a function is invoked for each of the frames.

The setAttributes() function

Correct me if I am wrong but this function appears to do the same thing that Object.assign(). I was able to replace calls to that function with calls to Object.assign() and still saw the attributes set as expected in Opera and Chrome.

Arrow functions with empty argument lists

You don't have to do this, but _ could be used instead of empty parentheses for arrow functions with no named arguments. Refer to this SO post and its answers for more context.

timeout in function returned by throttle()

While it would only save a couple lines, the arrow function passed to setTimeout() in the function returned by throttle() could be simplified to remove the curly braces. While this would mean that true would be returned, it doesn't affect anything.

const throttle = (fn, delay) => {
  let canCall = true;
  return (...args) => {
    if (canCall) {
      fn.apply(null, args);
      canCall = false;
      setTimeout(_ => canCall = true, delay);
    }
  };
};

1https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters)

\$\endgroup\$
  • \$\begingroup\$ yeah actually in the version I'm using it has a const readyInit = (() => {preloadPath();loadFrame();}); function I call on DOMReady. This was actually due to me showing a collage "I wish we could" and must have overlooked it when posting a copy+paste; good eye. I was not aware of the Lambda expression for empty ()! All valid and good points, thanks for your input! I'll update the OP to reflect changes when I get a moment. \$\endgroup\$ – darcher Dec 21 '18 at 17:12
  • \$\begingroup\$ Cool; Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. You could post a follow up question with update code... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 21 '18 at 18:55
0
\$\begingroup\$

I've updated the code based on the suggestions in comments and answers.

shout out to Sᴀᴍ Onᴇᴌᴀ for his suggestions in the accepted answer and David Knipe for suggestions on the throttle function.

const w = window;
const d = document;

let timeout;
const throttle = (fn, ...args) => (
  timeout && w.cancelAnimationFrame(timeout),
  timeout = w.requestAnimationFrame(_ => fn(...args)));

const loadFrame = _ => 
    d.querySelectorAll("iframe").forEach(frame => {
    if (frame.src) return
    const frameRect = frame.parentElement.getBoundingClientRect();
    (w.pageYOffset + d.documentElement.clientHeight >= frameRect.y
      && frameRect.y + frameRect.height >= w.pageYOffset)
      && frame.setAttribute('src', frame.getAttribute("data-src"))
              .removeAttribute("data-src")});

d.addEventListener("DOMContentLoaded", loadFrame, false);
w.addEventListener("scroll", _ => throttle(loadFrame), false);
\$\endgroup\$

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.