stimulusreflex / stimulus_reflex

Build reactive applications with the Rails tooling you already know and love.
https://docs.stimulusreflex.com
MIT License
2.28k stars 172 forks source link

SR strips out @click attribute from elements passed to selector morph #435

Closed shubik22 closed 3 years ago

shubik22 commented 3 years ago

Bug Report

Describe the bug

I came across this issue when passing HTML with an Alpine.JS @click attribute to a selector morph.

A simplified example:

morph '#my-el', "<div @click=\"modalOpen = true;\"></div>"

The HTML received by the client was "<div true></div>".

I believe this is due to these lines and the use of Nokogiri:

2.7.2 :007 > html = '<div @click="modalOpen = true;"></div>'
 => "<div @click=\"modalOpen = true;\"></div>"
2.7.2 :008 > fragment = Nokogiri::HTML.fragment(html)
 => #<Nokogiri::HTML::DocumentFragment:0x7c9c name="#document-fragment" children=[#<Nokogiri::XML::Element:0x7c88 name="div" attributes=[#<Nokogiri::XML::Attr:0x7c74 name="true">]>]>
2.7.2 :009 > fragment.to_html
 => "<div true></div>"

There's a pretty straightforward workaround for my situation (using x-on:click instead of @click), but I just wanted to file this bug as the behavior described above seems a bit unexpected and SR may want to support the use of those types of attributes.

To Reproduce

Call a selector morph with an @click attribute.

Expected behavior

The HTML received by the client should match what's passed to the selector morph serverside.

Versions

StimulusReflex

leastbad commented 3 years ago

Hi Sam!

Thanks for bringing this to our attention. 'coupla things:

  1. Strongly recommend that you update to SR v3.4.1 and CR v4.5.0 as they are really great, and there's no good reason to keep running a pre-release build.
  2. I don't know if you saw but we added a whole section on customizing CableReady#morph, with specific reference to Alpine: https://cableready.stimulusreflex.com/customization#shouldmorph-and-didmorph
  3. Nokogiri is a very particular black box, as is morphdom itself. Please don't read shade into this, but the reason it's not working is because you're giving it invalid HTML.

Now, I understand that "it's complicated" - this is the syntax pushed by libraries like Alpine, standards be damned. Here's the olive branch I can extend: if you can show me the functionally equivalent Nokogiri code that doesn't choke on this HTML-adjacent markup, I'll try hard to figure out a way to give you an escape hatch that you can call with an option.

shubik22 commented 3 years ago

Thanks for the detailed reply @leastbad, point taken about passing invalid HTML. Given an alternative solution exists (not using invalid HTML and using x-on:click instead), I'm happy to just close this issue and move on. The one thing I'm wondering is if you think it makes sense to note somewhere in the docs that the HTML passed to morph must be valid HTML? I'm happy to put out a PR doing that (maybe adding a note to the Morphing Sanity Checklist), please let me know what you think.

And I upgraded to the new SR/CR versions, thanks :)

leastbad commented 3 years ago

Right on. 👍

We can't actually accept documentation PRs due to silliness with our documentation system, but I will see if I can work in something about how HTML has to be HTML. 😉

shubik22 commented 3 years ago

Awesome, thanks so much.

leastbad commented 3 years ago

Hey Sam, I almost forgot: there's a non-zero chance that https://github.com/stimulusreflex/stimulus_reflex/pull/492 might have fixed this issue. YMMV! Clearly, you succeeded regardless. 🥇