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

restructuring syntax for rails 6.1 compatibility #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fneves
Copy link

@fneves fneves commented Oct 6, 2020

Rails 6.1 is introducing a check to verify if the class is the class is not a Singleton, and if it is raises a TypeError.
This PR simply changes the structure of the code and instead of calling mattr_accessor in the singleton class, simply calls it on the module. Making it ready for 6.1 or whoever is using the edge version of Rails.

@leastbad
Copy link
Owner

leastbad commented Oct 6, 2020

Thanks for bringing this to my attention! Can you explain why this check is performed? Why hate on singletons?

I'm actually very much hoping that Optimism won't be necessary on Rails 6.1, as DHH has said several times that remote form validations are "fixed" without providing details. All I know is that UJS is apparently going away and there is supposedly a better solution coming. Do you know anything about this?

@fneves
Copy link
Author

fneves commented Oct 6, 2020 via email

@leastbad
Copy link
Owner

leastbad commented Oct 6, 2020

For sure, and I really appreciate it.

I will try out the PR on my local machine and make sure this doesn't break.

@leastbad
Copy link
Owner

leastbad commented Oct 7, 2020

Hi @fneves, just wanted to give you an update.

Unfortunately, the solution you've offered fails if you're also using StimulusReflex, which makes this patch a non-starter. It effectively over-writes the channel SR uses with "OptimismChannel" ... which is bad.

However, we want to get this working so we're looking at other ways we could change this that won't break API compatibility. For example, it might benefit from actually making Optimism a real Singleton. I'm going to be testing various options today.

@fneves
Copy link
Author

fneves commented Oct 9, 2020

did you manage to get any solution?
I can help with something if you need to.
How did you test with StimulusReflex (I honestly don't use it so it never crossed my mind to test with it)?

@leastbad
Copy link
Owner

leastbad commented Oct 9, 2020

Sorry for the delay, this is a volunteer project so unfortunately it can sometimes take time for change to occur.

I have to figure out how to install edge Rails on my system in a way that lets me spin up new projects. I know it's probably basic, but I just haven't done it, yet.

As for StimulusReflex, installing it is very easy. Please give it a go: https://docs.stimulusreflex.com/setup

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

Successfully merging this pull request may close these issues.

2 participants