search

stimulusreflex / stimulus_reflex

Build reactive applications with the Rails tooling you already know and love.
https://docs.stimulusreflex.com
MIT License
2.28k stars 172 forks source link

SR doesn't seem to handle redirects / 302s #376

Closed romanos closed 3 years ago

romanos commented 3 years ago

From the docs:

The magic is that there is no magic. What the user sees is exactly what they will see if they refresh the page in their browser.

Sometimes, the next time a user refreshes the page, they are redirected for some reason. Either a condition has changed on the backend, a link has expired, etc. SR doesn't seem to handle this condition (though I could have sworn at some point in prior versions I was not seeing this behavior).

Instead, I'm seeing a blank page with the actual redirect text, "You are being redirected."

To Reproduce

I'm having this issue in a full app, but here's the minimum conditions to reproduce:

  1. Blank Rails app with SR

  2. Controller as such:

    class PagesController < ApplicationController
def one
if @next
  redirect_to '/pages/two'
end
end

def two
end
end

  3. Page one.html.erb containing: <a href="#" data-reflex="click->TestReflex#test">NEXT</a>

  4. TextReflex containing:

    def test
@next = true
end

Expected behavior

I would expect that when the server redirects me, that my SR-enabled page would redirect as well (or at least render the redirected page or fire a turbolinks.visit or whatever is appropriate). Instead, all content is wiped and I'm left with text saying "You are being redirected"

From the server log after click:

Processing by PagesController#one as HTML
Redirected to http://localhost:3000/pages/two
Completed 302 Found in 1ms (ActiveRecord: 0.0ms | Allocations: 148)

[ActionCable] Broadcasting to StimulusReflex::Channel: {"cableReady"=>true, "operations"=>{"morph"=>[{"selector"=>"body", "html"=>"You are being <a href=\"http://localhost:3000/pages/two\">redirected</a>.", "childrenOnly"=>true, "permanentAttributeName"=>"data-reflex-permanent", "stimulusReflex"=>{"target"=>"TestReflex#test", "args"=>[], "url"=>"http://localhost:3000/#", "attrs"=>{"href"=>"#", "data-reflex"=>"click->TestReflex#test", "data-controller"=>"stimulus-reflex", "data-action"=>"click->stimulus-reflex#__perform", "checked"=>false, "selected"=>false, "tagName"=>"A"}, "dataset"=>{"data-reflex"=>"click->TestReflex#test", "data-controller"=>"stimulus-reflex", "data-action"=>"click->stimulus-reflex#__perform"}, "selectors"=>["body"], "reflexId"=>"06811db4-5e71-4e8c-a1e9-21a588011fe2", "resolveLate"=>false, "xpath"=>"/html/body/a[1]", "cXpath"=>"/html/body/a[1]", "reflexController"=>"stimulus-reflex", "permanentAttributeName"=>"data-reflex-permanent", "formData"=>"", "morph"=>:page}}]}}

Versions

StimulusReflex

External tools

Browser

leastbad commented 3 years ago

I believe you! But we 100% need to see this in an MVCE because it's really odd only hearing this from one person and there's no way for us to debug it if we can't reproduce locally.

The easiest/fastest way is for you to clone https://github.com/leastbad/stimulus_reflex_harness or you can rails new and run the SR install pretty fast.

The other possibility is that something is misconfigured in your app. It's usually something to do with caching in dev mode without Redis. I want you to know that we will 100% do whatever we can to help, either way. Are you on Discord? If not, please consider joining us as we're quite responsive: https://discord.gg/XveN625

leastbad commented 3 years ago

Sorry, I actually just re-read the issue on my desktop and saw that you are literally using redirect_to in your controller for a page morph.

This doesn't work in SR, I'm afraid. It never has, and I suspect that it won't going forward.

I'm not saying that you can't use redirect_to in your application, but you can't do it when you're doing a page morph. When it is displaying "You are being redirected." that is because this is technically the new content of your page.

What I will do is add some words to the documentation to address this directly.

romanos commented 3 years ago

I suspected as much, but am retrofitting an existing app and this is the current behavior. Thanks for clarifying. Would be great to know the limitations or other conversion pitfalls in the docs. Thanks!

leastbad commented 3 years ago

Here's the thing: this isn't a limitation. SR is a tool for modifying the current page. SR has never followed 302 and indeed, it wouldn't make sense to given the goal of the library. If you need to morph to a new page, you should just navigate to that page.

StimulusReflex is designed to offer developers tools to create reactive functionality inside their UIs without having to use a Single Page Application framework. The key detail here being "single page" - whether you're morphing the entire page or just replacing the contents of a DIV, there's no concept of morphing one page such that it has the contents of another page, which lives at another URL.

As for improving the documentation, do you have any suggestions for what you would add? I'm biased but I feel like we actually go further than most projects when trying to anticipate the ways our users might get confused. There's always room for improvement, but I promise you we've already put in everything we've learned from answering questions for two years.

romanos commented 3 years ago

I understand the goal of the library, and I've had great success in 99% of contexts using SR. The thing is this:

If you need to morph to a new page, you should just navigate to that page.

Rendering a 302 on the server is the only way I know of to reliably redirect the browser to a new page from the server. I could obviously send a specific signal to the front end or use CableReady or render a javascript snippet or something else to change location, but especially in the context of third party libraries doing the rendering, it can quickly become cumbersome or impossible to do so without extensive patching.

Said differently (or more simplistically), it seems as though SR almost "disables" certain stock server responses from Rails that many of us have used for the past ten years without deviation. Perhaps there are perfectly reasonable reasons to do so, as you state above, but to the new SR users among us, it's an unexpected behavior that would probably benefit from some mention in the docs.

Just one person's opinion onboarding in real time. I'm having fun with SR otherwise.

leastbad commented 3 years ago

I hear you, and I appreciate you sticking with us.

It's true that CableReady#push_state is a thing, and that you can use CableReady#dispatch_event to accomplish just about anything with a tiny Stimulus controller listening for updates. These are the tools we reach for and prefer.

Aside from clarifying that, no, SR has never been able to follow redirects (a kind of gift in a world with so many uncertainties and ambiguities) what I'm really trying to explain is that SR is used almost exclusively with presentation URLS. By this I mean RESTful index and show actions, in addition to arbitrary routes people create.

The reason for this is that SR re-renders whatever comes up at that path.

Since you shouldn't ever mutate data with a GET request, there should be no danger of /posts/1 returning valid HTML and then responding to future GET requests to the same resource with a 302 redirect.

If /posts/1 is supposed to redirect to /posts/2, then the user should already be redirected to the new URL before SR comes into play. This is why I am insisting that SR isn't taking anything away from you.

If you have a situation where you're legitimately worried about redirects-as-side-effects, eg. a site where other people frequently set resources to redirect, you could add an after_update callback to your model and use CableReady to inform the client that such a redirect has occurred. Your Stimulus controller could receive an event from CableReady and use the History#replaceState method to update the user's URL and they might not even notice that the URL was changed.

So you see, you have several excellent options that are actually better than what Rails can offer. No need for a 302 in this scenario at all.

romanos commented 3 years ago

Again, I hear the point you're trying to make, but "shouldn't ever" is a very broad term. There are many authentication schemes that redirect on expired authentication or failed authorization. There are situations where a resource has moved or expired during session. More common, there are forms to be filled and edited in a wizard-like flow that ultimately end up in a redirection to a new asset. I understand it's not the most common use case, but it's not such a wild expectation that SR would mimic browser behavior and follow a redirect, especially when it's completely unambiguous.

To be clear, the solution is not what I'm after. I know how to get the result I want. It's the acrobatics I have to perform in order to do so that are surprising/frustrating. Especially when I'm integrating third party libraries over whose rendering schemes I have no control. I could monkey-patch or do any number of other things, but this is a common scenario and far from an edge case. I think "we don't do that" is not a sufficient response, fwiw. Just my two cents.

Thanks for taking the time on a weekend to hash it out with me.

leastbad commented 3 years ago

Paraphrasing this excellent thread on SE, "A GET should never change data on the server" is not advice, it's codified in the HTTP protocol. GET is supposed to be idempotent. It's not my opinion.

To join the rapidly growing demographic of SR enthusiasts, you will need to be open to achieving your end results a little differently, or else you're working against the grain of a tool that is carefully and intentionally designed. The learning curve isn't steep... if you give the tool a chance without projecting 2010 expectations on how it "should" work.

Step one on that path is acknowledging that SR operates on the current page. All morph concepts are based on replacing data on the current page. There is no concept of URL changing in SR because there is no concept of routing in SR. Make sure that your expectations are not liabilities preventing someone holding a wrench from seeing a ratchet.

What are these 3rd-party libraries that you are having trouble integrating with SR? And why are they redirecting GET requests without giving you any callback mechanism to respond to it?

SR is intended to be a strong alternative to SPAs, where people are typically not doing browser navigation redirects. The tooling SR and CR offer you to manage a change in state in between someone loading the page and refreshing the page is drastically more powerful than the tooling provided by vanilla Rails.

In Rails, If I'm looking at, making decisions based on, or entering data into a page based on the validity of the page that was rendered for me, and I hit a form submit on a now invalid resource, what's it going to do? A full page teardown and refresh, only to show me a flash error and probably throw away my now useless form state data. That was fine for 2010, but it's not what users want now.

Today, people want notifications that their document is dirty or there's some kind of error. They don't want to submit a real form and watch their browser load an entirely new page context. They definitely want early feedback on what they are doing so that you're not wasting their time.

I can't accept your claim of acrobatics. Using CableReady#dispatch_event in a model callback to tell a client-side Stimulus controller to replaceState really genuinely slick and elegant experience which you can pull off in a few lines of code. Meanwhile, redirect_to is simply not a good interface for the users we serve. Nobody would choose it over being automatically delivered to the correct document without even needing to understand that you've moved. Perhaps there's a notification, perhaps it just works without breaking your flow. Rails can't do that in 10 or 100 lines.

You've no doubt read the section in the Rails Doctrine on sharp knives. There are certainly folks who think in terms of request/response cycles with jQuery onLoad events and all of the things that make the typical React kid snicker at Rails. We're giving you a really sexy API that can deliver the same results as React, faster, with Rails. It will work with expiring sessions and has better tools for notifying you about stale data or forced logouts than could ever be possible with redirect_to.

We've been at this a really long time. It's just possible that maybe we've come up with a legitimately better way.

jonathan-s commented 3 years ago

I may be missing part of the discussion, but if you want SR to redirect in some way you can make that redirection happen in javascript in the stimulus controller after the reflex if it fulfills some kind of criteria.

romanos commented 3 years ago

Hey @jonathan-s, thank you for adding. I think we've moved on from the "how to do this" to the "why are things this way" part of the conversation. The how was never really under question, it's more about why.

@leastbad, you're reminding me about another aphorism about tools - when you're a hammer, everything looks like a nail. I understand your point of view, and I hear you on the evolution of better tools and user expectations. But you're taking a really dogmatic and somewhat inconsistent approach to how people should do things (not to mention who gets to be part of this community). You say "we have no concept of routing" but in the same breath talk about idempotency in the HTTP spec while ignoring 301/302s in the same spec. So yes, I understand it's not, in your view, the way things should be designed. But I live in a more pragmatic world in which I often have to integrate legacy systems. I'm trying to do right by my users and integrate better tooling, but it's not always as simple as throwing away and rewriting the entire application and its dependencies in order to upgrade a piece of it. My 2010 perspective may be mouldering at the edges, but perhaps there's a flipside to that as well.

Regardless, I'm all done. I think we've made whatever progress we are going to on this issue, and I thank you for your input. Feel free to have the last word on it. Thanks for all your work on this project.