Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent references to nodes swapped out of the dom accumulating as detached elements in memory #2091

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

croxton
Copy link
Contributor

@croxton croxton commented Dec 12, 2023

Description

I noticed that detached nodes accumulated in a project after multiple swaps. Memory heap snapshots contained detached elements with references to htmx-internal-data and a replacedWith property caused by this line:

Ref:

getInternalData(target).replacedWith = newElt; // tuck away so we can fire events on it later

Important notes:

  • I do not understand the purpose of the replacedWith property, there are no other references to it in the source code.
  • I am also deleting the element's htmx-internal-data in cleanUpElement() because some elements would retain content in this structure (with references to elements no longer in the dom) despite the preceding cleanup - I did not investigate why, or determine if there is ever a reason to retain data in htmx-internal-data when an element is "cleaned up".

Testing

Observed manually using Chrome's developer tools to take heap snapshots while performing a sequence of typical user tasks on a complex htmx application. And checked with Microsoft Edge which has a useful 'Detached Elements' feature. These changes prevented detached nodes accumulating.

Passes htmx automated tests.

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

@croxton croxton changed the title Prevent references to elements swapped out of the dom accumulating as detached elements in memory Prevent references to nodes swapped out of the dom accumulating as detached elements in memory Dec 14, 2023
@1cg 1cg merged commit 9052a4d into bigskysoftware:dev Dec 14, 2023
1 check passed
@1cg
Copy link
Contributor

1cg commented Dec 14, 2023

Thank you @croxton!

@grey4owl
Copy link

@croxton @1cg
Well, that's what broke my extension I was using this extension in my project and it worked really well but I didn't update htmx in a long time until now but after update it doesn't work anymore.. So i started to investigate and I found out that replacedWith doesn't exist anymore 😞 My project depends heavily on this extension and the reason why it worked so well is because I could replace the attributes of next target element before the request was executed, which I really needed so that it would not have problems later with alpinejs. So is there any workaround of this?

@croxton
Copy link
Contributor Author

croxton commented Sep 23, 2024

@grey4owl Looking at your extension it should be possible to store the element attributes you need before the swap happens on the htmx:beforeSwap event? Alternatively, there is an Alpine Morph extension for preserving the component state that you could try (I use this myself, it works really well): https://github.com/bigskysoftware/htmx-extensions/blob/main/src/alpine-morph/README.md

@grey4owl
Copy link

grey4owl commented Sep 23, 2024

@croxton It's not a problem to get attributes from target element that initiate request, it's a problem to get new target that is gonna be swapped, and with an old approach I didn't even have to do that because I could simply push those attributes that I want to preserve in target["htmx-internal-data"].replacedWith["attributes"]

@grey4owl
Copy link

grey4owl commented Sep 23, 2024

@croxton And yes I know that you can get new target after swapping and you can set new attributes but then you get alpinejs problems.. So I need a way to set custom attributes to new element that is gonna be swapped but before request is finished so that response be new element with those new attributes..

@croxton
Copy link
Contributor Author

croxton commented Sep 23, 2024

@croxton It's not a problem to get attributes from target element that initiate request, it's a problem to get new target that is gonna be swapped, and with an old approach I didn't even have to do that because I could simply push those attributes that I want to preserve in target["htmx-internal-data"].replacedWith["attributes"]

On htmx:beforeSwap, the event.detail.xhr.response contains the response before it is swapped in. You can get (and change if necessary) the new content that is about to be swapped in. E.g. if you wanted to parse the new content like you are querying a dom:

onEvent : function(name, event) {
      
  if (name === "htmx:beforeSwap") {
     let incomingDOM = new DOMParser().parseFromString(event.detail.xhr.response, "text/html");
     
     // Do stuff with incoming dom, such as get attributes from an element and cache them
     
     // This should work to modify the response before it is swapped, if you need to:
     event.detail.xhr.response = incomingDOM.body.innerHTML;
  }
}

@grey4owl
Copy link

@croxton I mange to make it work like you explained but i used event.detail.serverResponse since event.detail.xhr.response is "read-only". But is this the only way to make this? Somehow it feels like wrong way of doing this..

@croxton
Copy link
Contributor Author

croxton commented Sep 29, 2024

Great! Htmx's events are designed to allow listeners to mutate the objects passed to them, so I don't think it's wrong. You're extending htmx, exactly as intended.

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

Successfully merging this pull request may close these issues.

4 participants