phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.18k stars 930 forks source link

Missing beforeUpdated() hook for 3rd party js integration #559

Closed gregjopa closed 4 years ago

gregjopa commented 4 years ago

Environment

Actual behavior

The current JS hooks do not provide a way to clean up 3rd party JS components before LiveView manipulates the DOM. We have the phx-update="ignore" attribute to protect markup associated with 3rd party JS code. But phx-update="ignore" option does not prevent DOM manipulation for content inside lists.

This issue is particularly problematic for 3rd party js that uses iframes. Whenever an iframe or an ancestor of it are disconnected from the DOM, the iframe's contents will be cleared and the iframe will reload when it or its ancestor is reinserted back. For example, ad iframes can't be reloaded this way so they break and cause issues with memory management/garbage collection.

I did find a work around for executing JavaScript before LiveView manipulates the DOM. It's to add my own custom event listener to the form fields directly.

<form id="filters-form" phx-change="filter_change">
  <div class="filters">
    <label>
      <input type="checkbox" id="even" name="even">
      Filter results to even numbers only
    </label>
  </div>
</form>
mounted(){
  // create custom event listener to manually trigger the beforeUpdated() hook
  document.querySelector('input[type="checkbox"]').addEventListener('click', function () {
    Hooks.Ads.beforeUpdated();
  });
},

Expected behavior

A beforeUpdated() js hook available for properly cleaning up 3rd party JS code before any DOM manipulation takes place.

For example, here's what I'd like to be able to do for Ads:

Hooks.Ads = {
  mounted(){
    adModule.setupSlots();
  },
  updated(){
    adModule.setupSlots();
  },
  beforeUpdated(){
    adModule.destroySlotsInList();
  }
}

Example App

I forked the phoenix_liveview_example repo and put together a demo page to showcase this work around with integrating google ads into a LiveView app with a dynamic list. Here's the link to this demo: https://github.com/chrismccord/phoenix_live_view_example/compare/master...gregjopa:ads_in_list_demo. Note that the phx-update="ignore" attribute works great for the first ad since it's outside the list. The second ad is the problem since it's displayed in between the records in the list.

ads_in_list_demo_screenshot

Path Forward

I understand that this is technically a feature request. There is a work around but it does feel wrong. Other component frameworks like React and Glimmer.js expose hooks for executing JS before updating the DOM. My hope is for LiveView to provide the same.

chrismccord commented 4 years ago

Before I poke at this, it looks like you could put a hook directly on the ad row, then handle this in the destroyed callback. Can you confirm? Thanks!

gregjopa commented 4 years ago

@chrismccord thanks for the help with troubleshooting this issue! I tried out the destroyed() hook but that never executes for this use case regardless of which element I add the JS Hook to. The elements related to the ad in the list are never actually destroyed. Instead, the parent/ancestor element is updated which indirectly breaks the ad by causing the iframe reload issue.

For example, when the updated()/beforeUpdated() logic is commented out of the demo app (ex: https://github.com/gregjopa/phoenix_live_view_example/compare/ads_in_list_demo...gregjopa:ads_in_list_demo_mount_only) you can see that the ad in the list breaks but the actual ad markup is still in the DOM. Here's a gif to demo this behavior:

liveview_ad_reset_when_updating_list

Note how the ad iframe is still in the list after filtering the users to display only the even ones. The ask is for a beforeUpdated() hook that would run synchronously before the DOM is updated. That way the DOM related to this 3rd party ad can be safely cleaned up with gpt's method destroySlots()

I started poking at the LiveView js code in hopes of creating a PR for this behavior. But haven't had any luck so far. From what I can tell, the DOM is being updated before patch() ever executes. It seems like there is JS code manipulating the DOM after the checkbox is clicked and reseting the LiveView markup state before any diffing happens with morphdom. After testing this with the most recent version of LiveView, it does seem to be related to patch().

chrismccord commented 4 years ago

@gregjopa I've added a beforeUpdate and beforeDestroy hook callback. Can you try out cm-before-js branch? Assuming you have the following in your package.json:

    "phoenix_live_view": "file:../deps/phoenix_live_view"

You'll need to first point your mix.exs :phoenix_live_view dep at this branch, then:

$ mix deps.update phoenix_live_view
$ rm -rf assets/node_modules/phoenix_live_view
$ npm install --prefix assets

in that order. I still need to do a few more corner case evaluations on my side before merge since this was a bit of a refactor, but I would really appreciate a test drive. Thanks!

// @mcrumm

ccblaisdell commented 4 years ago

I understand that this is technically a feature request

A beforeUpdated hook is also really useful when doing FLIP style animations on elements that are owned by the liveview. I run into this issue often and look forward to trying out this branch! Thanks, Chris!

gregjopa commented 4 years ago

@chrismccord the cm-before-js branch is working great for me!

I updated the demo app to use it and can confirm that the new beforeUpdate() hook is running synchronously. When debugging the gpt.js ads I've verified that the timing is working as expected with destroySlots():

debug_ads_destroy_slots_timing

Also, thanks for fixing the bug around updated() firing on page load! 🎉