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

data-reflex-permanent not working when using slim templates #295

Closed ksweetie closed 4 years ago

ksweetie commented 4 years ago

Bug Report

Describe the bug

When I use erb files, the data-reflex-permanent attribute works great. However, if I use slim files, it seems these divs get updated regardless.

To Reproduce

Repro repo

bundle
yarn
bundle exec rails s

# localhost:3000/erb
# localhost:3000/slim

Expected behavior

erb and slim files should act the same. In the slim version, you can see that the React component gets blown away, as data-reflex-permanent is ignored.

sr-bug

Versions

StimulusReflex

External tools

Browser

leastbad commented 4 years ago

That is very strange, as you likely are aware.

First of all, thank you for preparing a reproduction repo. It's so much easier to help you this way. Kudos.

I'm distressed by how ugly Slim's output is. Haml was designed to output beautiful code. Just saying. 👍

Anyhow, the reason this is so disconcerting is that data-reflex-permanent handling is entirely client-side, so the template language shouldn't matter AT ALL. And yet, here we are.

I've just spent a good hour mucking with this, and I am completely stumped. What I can say is that the damage is being done on the first invocation of the reflex only; if you swap out the react_rails helper for = rand(1..100) you'll see the number change once and then stay consistent no matter how many times you increment.

Weirder still, when I run the erb version, inspect the div marked permanent and then grab the outerHTML, I can go to the slim version, instead that div and replace its outerHTML with the erb version. The first time you click increment, it blows away the value but then if I do it again, I can increment the number and the random number stays unchanged.

At the risk of sounding predictable, the first mitigation strategy is to not use Slim. I recognize that this might not be tenable for you, but it doesn't change the fact that it's true. We certainly are going to look into this but you do have at least one escape hatch.

rebuilt commented 4 years ago

Add an id attribute to the

data-reflex-permanent

declaration. i.e.

div { id="any-name" data-reflex-permanent}
ksweetie commented 4 years ago

Thank you for looking into this so quickly! I really appreciate it. Unfortunately, we're trying to add Stimulus Reflex to a large repo with many long slim files, so that isn't a great option.

However, I just tried adding an id to the permanent div, and to my surprise, it worked! I saw in the docs I needed an id for the element that initiated the reflex, but I didn't see anything mentioned about pairing data-reflex-permanent with an id. I assume it's still a bug, but that does unblock me at least.

Thanks so much to both of you! I'll leave this issue open, since there does seem to be an underlying bug.

leastbad commented 4 years ago

Trust me, I'm still stumped even while I'm thrilled that @rebuilt cracked the code.

I'm going to add a note to the documentation. Thanks folks.