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

Remove jquery ujs #1618

Merged
merged 1 commit into from
May 1, 2020
Merged

Conversation

nhippenmeyer
Copy link
Contributor

@nhippenmeyer nhippenmeyer commented Apr 22, 2020

rails-ujs has been moved into the rails project and using both jquery-ujs together with rails-ujs causes issues when attempting to programatically submit events (eg calling Rails.fire):

Screen Shot 2020-04-22 at 6 02 12 PM

@pablobm
Copy link
Collaborator

pablobm commented Apr 23, 2020

Thank you for the PR @nhippenmeyer. I'm trying to think if this could cause an unintended incompatibility; for example with Rails 5.0, as this change you mention only took place in 5.1 (if I have this right).

I ran a quick test with Rails 5.0, and it appears to work. I see that this version of Rails includes //= require jquery_ujs in its application.js, so that may be enough... but I'm not sure, as removing it (and using Administrate with your changes) doesn't break JS in an obvious way either.

This may require a bit more of investigation. I wonder if @nickcharlton has any thoughts?

@nhippenmeyer
Copy link
Contributor Author

Thanks for taking a look @pablobm. Could we add a deprecation warning for users on an older version of rails that this could potentially cause a breaking change? Happy to do more testing as well if that would be helpful.

@nickcharlton
Copy link
Member

I'm happy to go ahead with removing a dependency as long as it doesn't have any compatibility drawbacks.

I think at this point of Rails releases that if we had a warning for the last old version of Rails we support which helps users solve their problem easily it would also be a good tradeoff.

@nickcharlton nickcharlton added dependencies changes or issues relating to a dependency views-and-styles how administrate looks and is interacted with labels Apr 29, 2020
@pablobm
Copy link
Collaborator

pablobm commented May 1, 2020

I feel confident enough to merge this 🤞

@nickcharlton - Regarding your second point, I have created this issue to clarify Administrate's compatibility: #1631

@pablobm pablobm merged commit 15c49a6 into thoughtbot:master May 1, 2020
@pablobm pablobm mentioned this pull request Jun 25, 2020
4 tasks
nickcharlton added a commit that referenced this pull request Jun 26, 2020
In #1618, we removed the explicit include of `jquery_ujs` as it should
no longer be necessary. Alas, this broke the ability to destroy items.

This wasn't caught by the tests because Capybara tries to be clever with
these links, sending the `DELETE` directly instead of clicking on the link. We
disable this here and switch the specs to use the JS driver so that
we're actually testing the functionality (it can't pass without).

Adding the include of `jquery_ujs` solves this for now, unblocking us
from having a release for lots of other features and allows us to
revisit this problem again (in a way that we'll catch it this time!).

Fixes #1643.
pablobm added a commit to pablobm/administrate that referenced this pull request Jun 28, 2020
This reverts commit 15c49a6, which
broke the ability to delete records, as this relies on UJS.
@pablobm pablobm mentioned this pull request Jun 28, 2020
nickcharlton added a commit that referenced this pull request Jun 30, 2020
In #1618, we removed the explicit include of `jquery_ujs` as it should
no longer be necessary. Alas, this broke the ability to destroy items.

This wasn't caught by the tests because Capybara tries to be clever with
these links, sending the `DELETE` directly instead of clicking on the link. We
disable this here and switch the specs to use the JS driver so that
we're actually testing the functionality (it can't pass without).

Adding the include of `jquery_ujs` solves this for now, unblocking us
from having a release for lots of other features and allows us to
revisit this problem again (in a way that we'll catch it this time!).

Fixes #1643.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies changes or issues relating to a dependency views-and-styles how administrate looks and is interacted with
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants