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

Fix bug #5

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

Fix bug #5

wants to merge 1 commit into from

Conversation

fedealconada
Copy link

Fix following issue.

@schneidmaster
Copy link
Owner

Thanks for the CR. Can you also make the change in the source CoffeeScript? The dist folder gets overwritten when I build for a new release (also it's hard to review the change in minified JS).

@fedealconada
Copy link
Author

@schneidmaster hehe, I thought there were no sources files, don't know why. I've just added the changes!

@schneidmaster
Copy link
Owner

Hmmm. Your change doesn't seem to resolve the linked bug for me -- I get the same logged error. (I'm also hesitant to deviate from the official Rails source for actioncable itself.)

For what it's worth, I'm also strongly considering deprecating this library in favor of react-actioncable-provider. I haven't had time to keep this library up since I no longer use it, and react-actioncable-provider also uses higher-order components rather than mixins which is preferable since React discourages the use of mixins and it's much more future-proof. So you might also try to see if that library does what you need instead of sinking a lot of time over here. :)

@fedealconada
Copy link
Author

@schneidmaster it works for me :/, but it's up to you... I found it a bit more practical yours than react-actioncable-provider

@schneidmaster
Copy link
Owner

Yeah, sorry, I thought about it for a while and I'm going to deprecate this library. For the reasons I said above -- I don't have time to properly maintain it and there's another solution that's more current with best practices. Feel free to fork if you want to continue using it though :)

@fedealconada
Copy link
Author

fedealconada commented May 3, 2017 via email

@schneidmaster
Copy link
Owner

I tried a trivial case but haven't used it extensively -- I don't really have a need for this binding anymore in general. If you post your code with the new library I can see if I spot something.

@fedealconada
Copy link
Author

fedealconada commented May 3, 2017

@schneidmaster here I have it with your library. And I'm trying to put the other one but I couldn't... where can I reach you (like a chat, maybe skype)?

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