refactor: use anonymous function as event listener callback for listing meetings

[aqandrew]

Closes #6479

What changes did you make? / Why did you make the changes (we will use this info to test)?

See this issue comment for my analysis/recommendation.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

Screen.Recording.2024-03-27.at.11.11.54.AM.mov

Visuals after changes are applied

Screen.Recording.2024-03-27.at.11.12.55.AM.mov

[github-actions]

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b aqandrew-resolve-codeql-alert-6479 gh-pages
git pull https://github.com/aqandrew/website.git resolve-codeql-alert-6479

---

Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:

https://github.com/aqandrew/website/blob/resolve-codeql-alert-6479/CONTRIBUTING.md  

[ajb176]

ETA: 24 hours

[W1LD-R]

Review ETA: Wednesday 3/27 10pm CST

[W1LD-R]

Hey @aqandrew,

Your implementation overall looks correct and I would think it would work, but the data doesn't appear to be loading properly after I spin up my Docker container and go to localhost:4000. This is what I'm seeing on your branch:

I dug a little deeper and I think the insertEventSchedule function being called before the DOMContentLoaded event has occurred and eventData is available, but we want it to be called after.

The approach below worked for me. Can you try it and see if it works for you, too?

import { getEventData, insertEventSchedule } from "./utility/api-events.js";

/**
 * This type of function is called an IIFE function. The main function is the primarily controller that loads the recurring events on this page.
 * Refer: https://developer.mozilla.org/en-US/docs/Glossary/IIFE
 */
(async function main() {
  // Wait for DOMContentLoaded event
  await new Promise((resolve) => {
    document.addEventListener("DOMContentLoaded", resolve);
  });

  // Fetch event data
  const eventData = await getEventData();

  // Displays/Inserts event schedule to DOM
  insertEventSchedule(eventData, "events");

  // Displays/Inserts the user's time zone in the DOM
  document
    .querySelector("#userTimeZone")
    .insertAdjacentHTML("afterbegin", timeZoneText());
})();

This is what I'm seeing with the above adjustments:

[ajb176]

I'm seeing the same issue as @gaylem with the schedule not populating, nice catch.

I'm working on issue #5473 which faces the exact same issue with the same function on a different webpage. I looked through Andrew's analysis and we both came to the same conclusion that what was causing the error was passing a function with parameters to the event listener, which was causing the function to immediately run and try to pass its return value as the parameter. The function reference passed to addEventListener is supposed to be returnless, but because parameters were passed the EventListener assumes it's supposed to run the function and take its return value as its Listener.

What you mentioned @gaylem as the possible reason for the issue actually seems to be the way the current live website is operating and has been operating for a long time. The function which was supposed to wait for the DOM content to load before firing was just immediately running. At least that's what I assumed, I could be wrong. Andrew's solution was meant to fix that by using an arrow function. Thanks for taking the time to write an alternate solution, I'll try it out and also try to dig into why Andrew's solution isn't working, because I was going to approach the problem in a similar way. Maybe an issue with the this context changing?

I'll poke around and update because this issue and #5473 should be handled the same way.

[ajb176]

Here's my best guess, which could be wrong:

The original code causing the CodeQL error wasn't designed correctly and only worked because it didn't do what it was intended to do.

Because the script is asynchronous, it's possible that the event listener is set up after the DOMContentLoaded event, in which case it would be waiting for an event that already occurred and would never fire. In practice the live code incidentally works because the await operator loading the event data organically took longer than the DOM content loading, and once the script reached the event listener the function would immediately fire not because the condition was met but because it was loaded incorrectly to the event listener (with parameters).

Because Gayle's code immediately sets up the event listener, it does so before the DOM content fully loads therefore it doesn't miss the DOMContentLoaded event. The code also works if you remove the await block and event listener altogether, in which case it functionally operates the same way as the live code. I would suggest making a slight change to make sure the event listener missing the event isn't possible.

I have some code that seems to be working, I'll make a PR for #5473 in a couple hours and add you two as reviewers, because the issues are basically identical.

[ajb176]

Please see here

[W1LD-R]

Hey @ajb176, good job on that other issue! I was curious so just tested your approach on this issue and I'm still not seeing the data render. This is the code I used:

import { getEventData, insertEventSchedule } from "./utility/api-events.js";

/**
 * This type of function is called an IIFE function. The main function is the primarily controller that loads the recurring events on this page.
 * Refer: https://developer.mozilla.org/en-US/docs/Glossary/IIFE
 */
(async function main() {
  const eventData = await getEventData();

  //Displays/Inserts event schedule to DOM
  document.addEventListener(
    "DOMContentLoaded",
    function () { insertEventSchedule(eventData, "events") }
  );
  //Displays/Inserts the user's time zone in the DOM
  document
    .querySelector("#userTimeZone")
    .insertAdjacentHTML("afterbegin", timeZoneText());
})();

What a mystery! The only explanation I can think of as to why your solution works on your ticket but not here is that this code is pulling a literal ton of data and parsing it all out so that it's formatted properly on the client. Perhaps this is causing enough of a lag that we need to handle the promises with more care in this instance?

UPDATE: Just read your comment more closely and I think we're both saying basically the same thing lol. If there's a way to simplify my approach further, that works for me.

[ajb176]

Hey @gaylem, thanks for the review.

Could you re-check the code you quoted and what's being run, because it adds an anonymous function but doesn't have the if and else clauses to deal with the race condition of the DOM.

I adapted the code from the other PR here and it seems to be working on my end:

import { getEventData, insertEventSchedule } from "./utility/api-events.js";

/**
 * This type of function is called an IIFE function. The main function is the primarily controller that loads the recurring events on this page.
 * Refer: https://developer.mozilla.org/en-US/docs/Glossary/IIFE
 */
(async function main() {
  const eventData = await getEventData();

  //Displays/Inserts event schedule to DOM
  if (document.readyState==="loading") 
  {
  document.addEventListener(
    "DOMContentLoaded",
    function(){ insertEventSchedule(eventData, "events"); }
  );
  }
  else { insertEventSchedule(eventData, "events") }

  //Displays/Inserts the user's time zone in the DOM
  document
    .querySelector("#userTimeZone")
    .insertAdjacentHTML("afterbegin", timeZoneText());
})();

My code was based on yours, except I just added a check to make sure the event listener wasn't added too late, and an else clause to populate the schedule in case it was too late to catch the event.

[aqandrew]

Thank you both for being so thorough!

I believe you're both correct about race conditions between DOMContentLoaded and getEventData returning. I checked my original code after hard reloading, and saw a blank right column like @gaylem was seeing.

@ajb176 your version of this function works as expected! Even after hard reloading. 🙂 I'll update my implementation to match yours. It feels a bit wrong to have the exact same control flow shared across 2 files--although I would be happy to merge this PR (after revision) and #6543, then open up a new issue to create a utility function that both right-col-content.js and project-meetings.js can share. Thoughts?

[W1LD-R]

@ajb176 Ohhh got it, thanks for calling that out. My eyes must have been blurry.

@aqandrew Glad you got it working! And I like where your head is at re: creating a utility function. If you want to create an ER I'd be happy to add my two cents if you want them!

[ajb176]

Looks good to me!

I'd suggest updating the original post with a line mentioning the race condition as a reason for making the changes, just so things are clear for the merge team.

Sharing the control flow for the same task would be a good idea, thanks for taking the initiative to make the ER.

[W1LD-R]

Hooray @aqandrew -- looks great!

• You linked the issue correctly in your PR description
• Your branch is correct
• You clearly described the changes, stated the reason for making them, and provided before/after screenshots
• The data is loading and looks good across mobile and desktop on Chrome.

Nicely done -- Approved! 👍