How To Tell diffHTML To Remove A Script Tag
dyoder opened this issue · 17 comments
Please advise if this isn't the right place for this. The links on the Web site (Twitter, Reddit, …) didn't seem particularly active.
I have been using diffHTML for several years now. Works great. However, I recently came across a scenario that is causing me trouble. I am dynamically updating the head in a single-page Web app. In particular, the updated HTML doesn't contain a given script tag. So my expectation was that diffHTML would remove it. But it leaves it in place.
I can see that in many cases that's what you want. But in this case, I need to get rid of it. Is there a way to let diffHTML know that a given script tag is okay to remove?
Thank you for your work on this library and for any assistance you can provide.
To clarify: how can I let diffHTML know that a given dynamically added script tag is okay to remove?
Can you provide a reduced test case on Glitch or something similar? I've failed to reproduce this issue with existing or dynamic script.
Here is an example of diffHTML removing an existing script tag from head:
https://encouraging-nifty-bloom.glitch.me/
Here is an example of removing a dynamically added script:
diffHTML should have no opinion on scripts, and should remove them like any other element. If you're seeing inconsistent behavior, then that is definitely a bug and not intended. The only way that <script> and <link> tags are different is that their src and href attributes are treated as key to avoid unintentionally re-executing their contents if they change position in the DOM.
Here you go:
https://regal-frequent-motorcycle.glitch.me
A bit of explanation: I add a script that adds a script. (Specifically, the Google Analytics script.) diffHTML removes the initial script, but will leave the script that script added. I haven't tried to further simplify this to see if the issue is simply that the script is adding a script. But I can do that if you feel that would help debug the problem.
Thanks for the context and I'll look into this issue and try and reproduce into a unit test. What diffHTML does is cache the last known outerHTML of the element being rendered into and use that to diff at the next render cycle. If these differ at the start of a render diffHTML will recalculate the virtual dom to make comparisons stable. What might be happening is that the GA script gets added synchronously, diffHTML caches it via outerHTML, but does not represent it in the virtual dom. When we re-render, it is somehow gets lost and unaccounted for. I'd be very surprised if this is happening, but obviously something is wrong if we're producing a DOM that doesn't match the input. I wrote diffhtml-middleware-verify-state to help detect these sort of problems, so I'll give that a shot as well to see if it can help narrow down the source of the issue.
Thank you. And thank you again for this wonderful set of libraries. I am glad to know this is a bug and not some misunderstanding on my part. I had looked at the source and couldn't find anything other than the treatment of src as a default key (which is quite clever). FWIW, I did use the logging middleware (which confirmed that diffHTML did not include the script removal in the diff) but didn't try verify state.
My hunch was right and I wrote a unit test to show the issue:
it.only('will remove script tags which are dynamically added', async function() {
const { state } = await diff.innerHTML(this.fixture, `
<script></script>
`);
// Simulate a synchronous script being added.
const script = document.createElement('script');
script.setAttribute('id', 'test');
this.fixture.appendChild(script);
state.previousMarkup = this.fixture.outerHTML;
diff.innerHTML(this.fixture, ``);
assert.equal(this.fixture.innerHTML, '');
});-
-
- <script id="test"></script>When I remove previousMarkup lookups and always rebuild the tree per render the test passes. There will be a performance hit with this change. I'll open a PR and get a build available for you to test.
Given that there's a performance hit here, I wonder if perhaps this should be a configuration option. Although I'm unsure as to where that would be set.
I'm investigating if there is a better way of handling this problem. My initial assumption is that there isn't going to be a straightforward solution. What may need to happen is to defer script execution till after patching has completed, how to do this without tripping up CSP is going to be a challenge.
Hi @dyoder sorry it's taking so long to land a fix for this bug, but you stumbled on something that is worth fixing properly. I've introduced a new configuration option to enable/disable script's from running, and have a method in place to stop scripts from executing by default, and only trigger once rendering has fully completed.
Logic for script handling is now:
- Track all new script tags by their script type
- Set all new scripts type to 'no-execute'
- Render all elements to the DOM
- Reset the type to their original value
- Snapshot the DOM's markup for future comparison
- Execute all scripts if enabled
There is one more unit test to fix before I can land this, which is an edge case where you replace the top-level element you're rendering into with a script itself. I should have that fixed this week. Possibly as soon as Wednesday.
Thanks for the update! Your work to address this comprehensively is much appreciated. There is no rush on my account and please advise if there's anything I can do to help.
🎉
@dyoder I'll have a release out ASAP. Not sure why but lerna/npm/yarn have stopped working in this project locally. I can see Travis CI installed just fine, so I'm fairly certain it's something messed up specifically in my local env.
Will ping once it's out
v1.0.0-beta.18 is published which contains this fix. Let me know if you hit any issues, and thanks again for reporting!
I verified that https://regal-frequent-motorcycle.glitch.me/ correctly removes the script tag now 🎉
I'll let you know how it goes. Thank you for your work on this!
Works like a charm!