Skip to content

Comments

fix: head-support race condition#45

Open
stukennedy wants to merge 1 commit intobigskysoftware:mainfrom
stukennedy:main
Open

fix: head-support race condition#45
stukennedy wants to merge 1 commit intobigskysoftware:mainfrom
stukennedy:main

Conversation

@stukennedy
Copy link

@stukennedy stukennedy commented Jul 9, 2024 •
edited
Loading

Addresses the bug bigskysoftware/htmx#2599 ... raised against the original repo.

Description

currently the head-support feature starts doing its merging based off an htmx:afterSwap event. This means that any script being loaded into the DOM from the htmxSwap that wants to run some initialisation code may run after the htmx:afterHeadMerge event has triggered, and never see it.

An example for this would be if you want to return a ChartJS from an endpoint along with the script to display the chart.

    <head>
      <script src="https://cdn.jsdelivr.net/npm/chart.js"></script>
    </head>
    <canvas id="myChart"></canvas>
    <script>
      htmx.on('htmx:afterHeadMerge', function () {
        (async () => {
          while (!Chart) {
            await new Promise((resolve) => setTimeout(resolve, 100));
          }
          var ctx = document.getElementById('myChart').getContext('2d');
          var myChart = new Chart(ctx, { <CONFIG> });
        })();
      });
    </script>

The polling is necessary to check that the Chart class has been declared (i.e. the new head script has loaded)
(and yes this could do with a timeout so it doesn't try for ever, but this is the basic solution)
However, this code won't work every time if we have htmx:afterSwap in head-support.js as the trigger.

Changing htmx:afterSwap to htmx:afterSettle ensures the DOM has settled (i.e. our event listener has been registered) before the head swap functionality is performed and our code works as expected.

Testing

This was tested with the above code by calling this code on a lazy-loaded hx-post.
The race condition seems to be gone.

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

@netlify
Copy link

netlify bot commented Jul 9, 2024 •
edited
Loading

✅ Deploy Preview for htmx-extensions canceled.

Name Link
🔨 Latest commit dfba1c0
🔍 Latest deploy log https://app.netlify.com/sites/htmx-extensions/deploys/668ce427feba6b00086887d1

@stukennedy stukennedy mentioned this pull request Jul 9, 2024
Closed
4 tasks
@Telroshan Telroshan added the bug Something isn't working label Jul 9, 2024
@Telroshan
Copy link
Collaborator

Telroshan commented Jul 23, 2024

Hey, to prevent any future regression and be 100% sure that the fix works as expected, could you add a test case for this ? To first reproduce the error, then ensure this fix resolves it
From the top of my head, I know we can mock timeouts in the test suite (at least the main repo has some of these in its test suite), and you can configure the settle delay of htmx using its configuration property, so I would expect it to be doable!

PS: I'll shamelessly copy-paste that comment to the v1 PR on the main repo for the sake of clarity/history

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working needs test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stukennedy @Telroshan