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

Custom Stimulus schema breaks Reflex #91

Closed nickyvanurk closed 4 years ago

nickyvanurk commented 4 years ago

Describe the bug

Reflex ignores the schema set in Stimulus. It assumes Stimulus will always use the default schema.

Line 220 in javascript/stimulus_reflex.js

 if (actionValue) element.setAttribute('data-action', actionValue)

Hardcodes the 'data-action' attribute.

In our project we use a custom schema as follows:

const application = Application.start(document.documentElement, {
  controllerAttribute: 'data-controller',
  actionAttribute: 'data-do',
  targetAttribute: 'data-target'
});

Because we had a conflict with another plugin, cropperjs.

Feedback would be appreciated.

hopsoft commented 4 years ago

Great report. We need to update Reflex to support custom Stimulus schema.

leastbad commented 4 years ago

Is the schema object available easily through the application scope? If so, you could probably pass the current value right into line 220.

I hope all blockers are this easy to patch going forward. 😀

nickyvanurk commented 4 years ago

After reviewing both Stimulus and Reflex it can probably be fixed by simply changing 'data-action' to:

application.schema.actionAttribute

I tried creating a pull request but I am not allowed to, kinda new to open source contributions.

Adding a reflexAttribute to the schema would also be nice so 'data-reflex' can be customized.

EDIT

Adding it to the Stimulus config schema does nothing to Stimulus so you could use that as a place to store the custom Reflex attribute:

const application = Application.start(document.documentElement, {
  controllerAttribute: 'data-controller',
  actionAttribute: 'data-do',
  targetAttribute: 'data-target',
  reflexAttribute: 'data-flex'
});

Then call it in the Reflex code as:

application.schema.reflexAttribute

There should be a default set in the Reflex code though otherwise it would break the code if it isn't set in the Stimulus schema config (because the Stimulus schema config is optional).

leastbad commented 4 years ago

Welcome to the party, @nickyvanurk. I like you already.

What kinds of things are you hoping to work on with StimulusReflex? Is there anything else that we can do to help, after we get this schema issue sorted out?

leastbad commented 4 years ago

To your question about pull requests, StimulusReflex forced me to finally figure out the contribution workflow. Happy to help off-line if you want to DM me, but the basic idea is that you fork the project to your own account, make a feature branch, change that feature branch, commit it, push it, and when you push it, GitHub will prompt you to open a PR from your feature branch back to hopsoft:master. And then we all win!

nickyvanurk commented 4 years ago

Thank you for the kind words @leastbad I am implementing a private chat feature. I was doing the things Reflex does by hand but then a colleague of mine told me about the Reflex gem.

nickyvanurk commented 4 years ago

I made a pull request, two simple changes in the code. That should fix the issue.

leastbad commented 4 years ago

Make sure that you check out the chat example on the StimulusReflex Expo site!

There is a feature PR on hold waiting for a real use case. Could you peek at #87 and tell us whether that would be useful to you?

Providing an alternative schema for data-reflex is a lot more involved but still probably possible. It’s a lot more prevalent through our codebase for obvious reasons, but there’s further issues because we have additional attributes like data-reflex-permanent that have (currently) hard-coded server-side strings in the Ruby code. It’s hard to imagine how we could accommodate this without passing a lot of meta-metadata with each call.

We’ll have to talk about this. Changing the Stimulus schema is a thing and action is a much more common term than reflex, which we’ve never suggested people could change.

Lots to think about.

leastbad commented 4 years ago

... we could allow people to define an initializer that sets these strings, @hopsoft. 🤔

nickyvanurk commented 4 years ago

@leastbad #87 Is actually what I was already looking for! Would be really useful in my chat feature for appending new conversations and messages.

leastbad commented 4 years ago

Not paying this guy, @hopsoft. I swear! 🤓

nickyvanurk commented 4 years ago

Any indication when this enhancement will, if possible, be accepted? Manually building the gem and including it in my project doesn't seem to work. Also, in my gemfile when I use

gem "stimulus_reflex", git: 'https://github.com/nickyvanurk/stimulus_reflex', branch: 'stimulus-custom-schema-support'

you would think it will work, right? But when I inspect the stimulus_reflex.js in my browser while the project is running it still shows the old line of code (line 220). Any feedback would be appreciated.

EDIT

Alright I fixed it, for now. I edited the stimulus_reflex.js manually inside the node_modules directory. I am probably missing something on how to reinstall reflex because the reflex folders in the node_modules don't go away when I remove the gem in my gemfile. I am still fairly new to the Ruby/Rails world.

leastbad commented 4 years ago

Usually what you do is go to your project root folder and run yard link stimulus_reflex (assuming you’re using yarn). If you’re still seeing the old js, go into node_modules, delete stimulus_reflex, run yarn link again and then finally, restart webpacker via bin/webpack or bin/webpack-dev-server

Let us know how things turn out. Good luck!

nickyvanurk commented 4 years ago

@leastbad Is there anyway we can communicate outside of GitHub? Could use some input on my current project :) You can also email me, my mail is listed on my profile.

ghost commented 4 years ago

@leastbad Is there anyway we can communicate outside of GitHub? Could use some input on my current project :) You can also email me, my mail is listed on my profile.

Hey @nickyvanurk please have a look the following issue: #90 Stimulus Reflex has a Discord server where you can chat real-time.

You can join here: https://discord.gg/XveN625