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 remote.js #4030

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

🔥 Remove remote.js #4030

wants to merge 8 commits into from

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Mar 4, 2025

TL;DR

image

This was all along supported by rails!!!

Description

Intro

Welcome to 10 Sept, 2010. A world of Rails 2, committing changes without a PR and apparently... no UJS?.
https://github.com/3scale/system/commit/d40a74950775f34b28eeb455a56ace1f078504b6

remote.js (previously belonged to threescale.application.js) emulates rails-ujs functionality by adding class remote to forms and links. Instead of sending a synchronous html request, with page reload, they will expect an asynchronous javascript response from the controller.

On 29th August 2010, rails 3.0 was released with the following new feature:

Unobtrusive JavaScript helpers with drivers for Prototype, jQuery, and more coming (end of inline JS)

Using attribute [data-remote] in links and forms.

Why did the wise people of 2010 decide not to use UJS is unknown. But why we have been maintaining this up until 2025, being already a builtin functionality of rails is beyond reckoning.

Study case: Plan New Feature

Screenshot 2025-03-07 at 09 20 51

By clicking "New Feature" a modal is opened with the following "remote" form:

<form class="remote formtastic feature" id="new_feature" action="/apiconfig/plans/7/features" accept-charset="UTF-8" method="post">
  ...
</form>

Screenshot 2025-03-07 at 09 21 00

Upon submission, the form will trigger an AJAX call to this method:

def create
@feature = collection.build(feature_params.merge(scope: @plan.class.to_s, featurable: @plan.issuer))
respond_to do |format|
if @feature.save
format.js
else
format.js { render :action => 'error' }
end
end
end

And then return:

var html = "<%= escape_javascript(render :partial => 'feature', :locals => {:feature => @feature}) %>";
jQuery.colorbox.close();
jQuery("table#features tbody").append(html);
jQuery("table#features tr.notice").hide();
jQuery.flash.notice('Feature has been created.');

Which will update the current page without reloading:

Screenshot 2025-03-07 at 10 05 25

When class remote is not present, a :js response from the server will look like this in the browser:

Screenshot 2025-03-07 at 09 21 05

The Twisth

Replace class remote with prop remote: true and voilà, it behaves exactly the same.

<form class="formtastic feature" id="new_feature" action="/apiconfig/plans/7/features" accept-charset="UTF-8" data-remote="true" method="post">
  ...
</form>

Verification

Check pre-existing forms and anchors with class remote.

Places already found and verified (by @josemigallas):

  • Plan: New feature (Modal)
  • Plan: Edit feature (Modal)
  • Plan: Pricing tab
  • Plan: New pricing rule (Modal)
  • Plan: Edit pricing rule (Modal)
  • Plan: Limits tab
  • Plan: New usage limit (Modal)
  • Plan: Edit usage limit (Modal)
  • Invoice: New line item (Modal)
  • Account detail: Send message (Modal)
  • Remote fancy buttons
    • Plan's delete pricing rule
      app/views/api/pricing_rules/_pricing_rule.html.erb:10
    • Invoice action buttons
      app/views/finance/provider/invoices/_actions.html.erb
    • button_activate_or_suspend
    • button_to_toggle_suspend_buyer_user
    • remote delete_button_for
      • Delete plan feature
      • Delete plan usage feature
      • Delete referrer filter
    • action_button_to

Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I verify this works fine?

@josemigallas josemigallas changed the title Update remote.js 🔥 Remove remote.js Mar 7, 2025
@mayorova
Copy link
Contributor

mayorova commented Mar 7, 2025

Great description @josemigallas !

But looking at the PR, does it mean that we keep maintaining this remote.js functionality? 😅

@josemigallas
Copy link
Contributor Author

How can I verify this works fine?

I was looking for a good example and started a bit of archeology... Read the updated description 😂

@josemigallas
Copy link
Contributor Author

Great description @josemigallas !
But looking at the PR, does it mean that we keep maintaining this remote.js functionality? 😅

It's a WIP

@josemigallas josemigallas changed the title 🔥 Remove remote.js 🚧🔥 Remove remote.js Mar 7, 2025
@josemigallas josemigallas marked this pull request as draft March 7, 2025 11:14
@josemigallas josemigallas changed the title 🚧🔥 Remove remote.js 🔥 Remove remote.js Mar 7, 2025
@josemigallas josemigallas marked this pull request as ready for review March 10, 2025 09:30
akostadinov
akostadinov previously approved these changes Mar 10, 2025
Copy link
Contributor

@akostadinov akostadinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Seems like a debug change needs to be removed though, before merging.

@mayorova
Copy link
Contributor

This has a side effect in some forms, well, I've seen one so far:

image

Apparently, because of this style:

#actions form.button-to.remote {
@include white-box-shadow;
display: block;
padding: line-height-times(1);
}

@josemigallas
Copy link
Contributor Author

This has a side effect in some forms, well, I've seen one so far:
image
Apparently, because of this style:

#actions form.button-to.remote {
@include white-box-shadow;
display: block;
padding: line-height-times(1);
}

Ah! Good catch. I certainly didn't look in the css

Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only tested the invoices screen and works fine. The code looks good.

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.

4 participants