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

form_with should default to local: false #39711

Closed
Ecco opened this issue Jun 24, 2020 · 14 comments · Fixed by #40708
Closed

form_with should default to local: false #39711

Ecco opened this issue Jun 24, 2020 · 14 comments · Fixed by #40708

Comments

@Ecco
Copy link
Contributor

Ecco commented Jun 24, 2020

When creating a form with form_with, the localattributes defaults to true. That is a problem because it forces the unsuspecting user to handle XHR requests.

I think it would be OK to make local: true the default value if it automagically worked out of the box. But it doesn't, and actually it breaks in very weird ways: the form doesn't seem to respond, even though a request hits the server. One need to notice that the format is set to JS, which is very non-obvious.

As a matter of fact, even a new scaffold will have to specify local: false, which I think proves my point.

Steps to reproduce

rails new FooBar
rails generate scaffold Article title:string body:string
head -n 1 app/views/articles/_form.html.erb

Expected behavior

<%= form_with(model: article) do |form| %>

Actual behavior

<%= form_with(model: article, local: true) do |form| %>

System configuration

Rails version: 6.0.3.2
Ruby version: 2.6.6p146

@composerinteralia
Copy link
Member

Thanks for the issue. What you are saying seems reversed to me.

From the documentation for form_with:

:local - By default form submits are remote and unobtrusive XHRs. Disable remote submits with local: true

So the the scaffold generator builds a form that has XHRs disabled by default:

<%= form_with(model: article, local: true) do |form| %>

To enable XHR you would need to change the generated local: true option.

If you are seeing something different, could you offer a way of reproducing that? I followed the steps you listed above and I was seeing the behavior I expected with normal, non-XHR form submission.

@Ecco
Copy link
Contributor Author

Ecco commented Jun 26, 2020

Hi @composerinteralia ! I'm really sorry, my explanations weren't very good indeed. I'm going to start over, hopefully it's clearer this time!

I probably should have started by saying that the code generated by a scaffold works perfectly. Otherwise a bunch of people would have probably noticed before me 😄

That being said, I think the code generated by a scaffold is a proof that form_with's defaults are not very well chosen. Indeed, if a scaffold has to specify local: true, then why is that not the default value?

Currently, to get a form generated with form_with(@mymodel) to work, you have to either:

  1. Specifically handle XHR requests and responses, which is a lot of work
  2. Add an extra parameter, namely local: true in your call to form_with

Both of those options require some extra work, which is why I think neither is good. Currently the scaffolding went for option 2, which is a lot simpler than 1. That's a fair choice, but I think there's a better 3rd option: making local: true the default in Rails since the current one (local: false) has no chance of working out of the box anyway.

@composerinteralia
Copy link
Member

Aha, now I am understanding you.

form_with was introduced in #26976, and the comments show that enabling XHR by default was an intentional decision.

The scaffold generator switched over to using form_with in #28406. I wonder if we added local: true there to preserve the old behavior of the generator.

I agree it is awkward to have Rails generate a form with non-default options.

@Ecco
Copy link
Contributor Author

Ecco commented Jun 26, 2020

I'm glad my explanations were better this time. Thanks for pointing to the relevant PRs!

I believe the second PR makes sense: it's great that scaffolds use the latest and greatest features. That's cleaner, and it's a way to spread awareness of newer features.

I wonder if we added local: true there to preserve the old behavior of the generator.

Well, I think local: true was added there because the other alternative would have been far more involved. The generator would have needed to generate fully working Ajax forms, and that's a lot of work!

That why I think the decision made in the first PR was suboptimal. Don't get me wrong, I think Ajax forms are great. But getting them to work requires a lot more work, at least at the moment. So until they work out of the box in Rails (if they ever should), I think form_with should default to local: false.

I feel like I have somewhat convinced you, hopefully other people will agree too!

@composerinteralia
Copy link
Member

composerinteralia commented Jun 26, 2020

I ran

rails new FooBar
cd FooBar
bin/rails generate scaffold Article title:string body:string
bin/rails db:migrate
bin/rails server

then edited the form to remove local: true and the Ajax form submission worked fine for me out of the box.

What sort of problems are you having? I have used remote forms on several projects now, and I don't recall needing to do any complicated setup.

@Ecco
Copy link
Contributor Author

Ecco commented Jun 26, 2020

You're right, your example does work. Thanks for taking the time to run it by the way! But it works only because it's a tiny bit too simple. As soon as there is any validation error on a model, it will stop working. For example, try to edit app/models/article.rb to:

class Article < ApplicationRecord
  validates :title, format: /foo/
end

Now go ahead and try to validate the form with a title that does not contain "foo". If you leave local: true, everything works great, with proper error messages and highlighting of erroneous fields. But if you remove it… it's totally broken!

@composerinteralia
Copy link
Member

composerinteralia commented Jun 26, 2020

Ah, right. Now that you are saying that, it does sound familiar. @seanpdoyle turned me onto turbolinks/turbolinks#85 (comment). Adding that snippet of JS is enough to get your errors rendering again.

Anyway, I see your point that it does require some additional setup. @kaspth you probably have opinions here (about defaulting to remote forms in form_with, but generating forms with the local: true in the scaffold generator). Is there a better place to have this conversation? I know we try to keep to bug tracker focused on bugs, and I don't know if this falls exactly into that category.

@Ecco
Copy link
Contributor Author

Ecco commented Jun 26, 2020

Another interesting thread you just found! Apparently I'm not the only one thinking this default should be changed. @jorgemanrubia said:

The fact that forms in scaffolds are generated with local: true in Rails 5.2 looks like an smell to me. The vanilla code generated by Rails won't use its own defaults!

@rails-bot
Copy link

rails-bot bot commented Sep 24, 2020

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-0-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Sep 24, 2020
@Ecco
Copy link
Contributor Author

Ecco commented Sep 24, 2020

This issue is definitely not stale and still very much present…

@rails-bot rails-bot bot removed the stale label Sep 24, 2020
@SteveRawlinson
Copy link

I barely feel qualified to comment on this but I too think it's very odd that you have to specify a non-default option in order to get forms working properly without a lot of extra work.

@jhackett1
Copy link

this is my biggest problem with rails at the moment - such a frustration and very much anti-"convention over configuration"

@silviorelli
Copy link

It feels just strange that, by default, the response after the submission of a form with validation errors will not be displayed when using turbolinks (also enabled by default), especially in a framework that's all about good practices and convention over configuration.

Turbolinks does not handle the render :edit that's the usual fallback of an update action when there are validation errors.

See: #34839

@Ecco
Copy link
Contributor Author

Ecco commented Dec 1, 2020

Sweet! Thanks for fixing this!!

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 a pull request may close this issue.

5 participants