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

Action form error handling when no JS/WASM #2110

Closed

Conversation

SleeplessOne1917
Copy link
Contributor

This is a WIP PR for #1891. I am opening this PR to make collaborating (with @gbj and any other contributor interested in this feature) easier. So far I am having trouble figuring out how to make the ActionForm component decode the error from the URL when it is rendered on the server.


if let Some(mut url) = url {
url.query_pairs_mut().append_pair(
"server_fn_error",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to use a key that matches specific server functions before merge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this should be fine, especially if create_server_action is responsible for checking its particular error in the URL, because it knows the server fn's unique name already. (Could use the ServerFnType::url()` or some form of it as the key)

@gbj
Copy link
Collaborator

gbj commented Dec 22, 2023

Thanks for your work on this. In terms of substance it looks to me like the plan is to serde_qs encode the error and pass it in the URL of the redirect as an additional query param, right? I don't see any handling for that yet but maybe it should be baked into create_server_action, so that the action starts with its .value() as the error if one is included in the URL?

(I'm fine with the stylistic changes except the spelling of "referrer" in the comment. Two Rs is correct, one R as in HTTP is a typo that became baked into the spec 😄)

@SleeplessOne1917
Copy link
Contributor Author

(I'm fine with the stylistic changes except the spelling of "referrer" in the comment. Two Rs is correct, one R as in HTTP is a typo that became baked into the spec 😄)

You learn something new every day. I'm hoping my working on the other integrations make what I'm trying to do a bit clearer.

@gbj gbj mentioned this pull request Jan 4, 2024
20 tasks
@SleeplessOne1917
Copy link
Contributor Author

@gbj I've made a bit more progress. I found out the way to distinguish serverfns. The roadblock I'm at currently is finding a way for the browser to let server know that it has an error. This diagram is what I have in mind:

image

The diagram mentions a custom header. I suspect it would be possible to do away with the custom header if there is a way to read the referrer of a page request, parse server function errors from the query string, and match them to any action forms that get rendered on the returned page. My first thought of how to accomplish this is some sort of middleware, although I'm not sure where in the codebase to handle that. I would appreciate any suggestions for files or modules to investigate.

@gbj
Copy link
Collaborator

gbj commented Jan 13, 2024

@SleeplessOne1917 Thanks so much for your work on this and apologies for not replying sooner. Sometimes it takes me a while to find time to sit down and work on something large all at once!

I've also been in the process of porting over our server function rewrite for the upcoming release (#2158) so I wanted to get that in shape first.

I think your work on this is basically right on. I took inspiration from what you were doing here and managed to implement a working version (I think?) over on the other branch, using many of the same types but adapted.

Here you can see it working with/without WASM enabled.

Screen.Recording.2024-01-12.at.8.40.21.PM.mov

A few limitations:

  • It only supports a single error. I think this is probably correct, because it should only be used when you hit a server function endpoint, so it can only generate a single Err(_) per request anyway.
  • It identifies errors by server fn, not by action. If you have multiple actions that separately access the same server fn, it will use the same initial error state. I think that's probably fine... as in, it's probably a graceful enough degradation for that edge case in the no JS/WASM condition. But I'm open to changing that.

@SleeplessOne1917
Copy link
Contributor Author

In the video you posted, it seems that successes don't work for the no-JS/WASM case. I'm gonna keep working on this for a bit.

@gbj
Copy link
Collaborator

gbj commented Jan 13, 2024

Oh, is the intention to change the behavior if successes as well? I guess that makes sense, although I can't recall anyone having requested it before.

@SleeplessOne1917
Copy link
Contributor Author

Oh, is the intention to change the behavior if successes as well?

No. However, it handled successful submits successfully and error submits unsuccessfully in main. Your demo demonstrated the two being swapped (success doesn't work, error does work). I'm fairly sure it's a bug.

@gbj
Copy link
Collaborator

gbj commented Jan 13, 2024

I must be misunderstanding something. I don't believe server actions currently indicate any kind of success if they are successful with no JS/WASM, they simply redirect back to the referrer. Any indication of success is because new data are being loaded from a resource that shows the new server state.

@SleeplessOne1917
Copy link
Contributor Author

Any indication of success is because new data are being loaded from a resource that shows the new server state.

That's what I mean. If it succeeds with no WASM/JS, the HTML that gets rendered on the server should use that value. If you notice when you submit with the checkbox unchecked in your video, the text "Successful submit" doesn't get rendered above the form. That text should be rendered on the server and returned with the rest of the page's HTML.

@SleeplessOne1917
Copy link
Contributor Author

SleeplessOne1917 commented Jan 13, 2024

@gbj My most recent commit is getting pretty close to what I was aiming for (with the actix integration at least). I've run into a bit of trouble that your knowledge of the reactive system will be helpful for.

Currently, when JS/WASM is disabled, the action value gets updated successfully for both successes and errors. This can be verified by watching the server side logs when running cargo leptos watch and submitting the form with JS/WASM disabled. You will see a message that says "Got value = {value here}", which is coming from the isomorphic effect in this PR's example project. This message would not have the value if it was not being fetched as expected. Despite this, the result doesn't get rendered during SSR.

In short, a signal update is triggering an effect to rerun, but is not used in rendering for reasons unknown to me.

Edit

I've discovered that the value signal renders when it's a child of the ActionForm or appears after the ActionForm, but not when it is outside the action form and before the action form.

@benwis
Copy link
Contributor

benwis commented Jan 13, 2024

So it seems to me the idea is to encode the response to a server fn, whether success or failure, in the url so that it is automatically extracted by ActionForm(unless it's a redirect) when JS/WASM is disabled.

The new version of server fns can return a pretty wide array of non-serializable data responses, so we'd have to limit this to a particular set of combinations

@gbj
Copy link
Collaborator

gbj commented Jan 13, 2024

Any indication of success is because new data are being loaded from a resource that shows the new server state.

That's what I mean. If it succeeds with no WASM/JS, the HTML that gets rendered on the server should use that value. If you notice when you submit with the checkbox unchecked in your video, the text "Successful submit" doesn't get rendered above the form. That text should be rendered on the server and returned with the rest of the page's HTML.

When I say "loaded from a resource," I mean "loaded from a resource (created using create_resource and read under <Suspense/>). Your example just uses an action and its value. Its behavior on the main branch (without JS/WASM) is that there's no indication of success on success, and it lands on the error message and stays there on an error. The design has been that resources are for loading data and actions are for mutating data. So typically (as in the todo app examples) a successful no-JS action is reflected in, for example, seeing the new todo.

It's sometimes nice to indicate successful mutations, too. (UI wise something like a message "Your changes have been saved" or whatever.) But as Ben points out the results of server functions can be arbitrarily complicated and not necessarily encodable in the URL. Even now, in 0.5, a server function could return some CBOR-encoded sequence of bytes. Whereas the error message is alway stringifiable.

So I'm just saying that the solution I showed on the 0.6 branch is a strict improvement over main and solves the problem as it's always been formulated.

Including the success state would probably require either 1) just setting a "was successful" boolean signal without the whole value or 2) a more restricted type that requires a stringifiable result.

I've discovered that the value signal renders when it's a child of the ActionForm or appears after the ActionForm, but not when it is outside the action form and before the action form.

You are setting the value in an effect inside the form, rather than setting the value when the action is created, which means that it isn't updated until the form component runs. SSR goes in a single pass, so setting a signal in an effect further down the tree won't rewind and start rerendering an earlier chunk of HTML, so it won't change a value that's already been rendered.

@SleeplessOne1917
Copy link
Contributor Author

I failed to consider that successful responses wouldn't always be serializable. Only returning the errors in the URL sounds good enough for me. IIRC, the main branch doesn't even need to serialize the success responses to get the HTML to render correctly on the server. This makes me think the behavior I pointed out in previous comments is still a regression. I'll revert the last few commits I made where the success serializable and see if I can get successes working like they originally did.

@benwis
Copy link
Contributor

benwis commented Jan 13, 2024

I failed to consider that successful responses wouldn't always be serializable. Only returning the errors in the URL sounds good enough for me. IIRC, the main branch doesn't even need to serialize the success responses to get the HTML to render correctly on the server. This makes me think the behavior I pointed out in previous comments is still a regression. I'll revert the last few commits I made where the success serializable and see if I can get successes working like they originally did.

If you're looking to display a success message, gbj is right that you'd want to have a resource on the page to get that value, which definitely can be displayed in the html with any of the branches(and if it isn't its a pretty big bug). With JS/WASM enabled, the action returning should trigger a resource refresh(and view update) without a page reload. With JS/WASM disabled, it'll redirect to the page and get the current state from the resource.

At least that's where my mental model lies. Having errors be returned and parsed from the query string is a definite improvement when there's no JS/WASM

@SleeplessOne1917
Copy link
Contributor Author

@benwis I'll give the resource tactic a try. Expect some more commits later today.

CC. @gbj

@SleeplessOne1917
Copy link
Contributor Author

I kind of messed up and got my git history in a bad state. If the stuff on @gbj 's form from the earlier stuff in PR is still around, that should be sufficient.

@benwis
Copy link
Contributor

benwis commented Jan 14, 2024

I kind of messed up and got my git history in a bad state. If the stuff on @gbj 's form from the earlier stuff in PR is still around, that should be sufficient.

I wouldn't worry about it, server fns are already going through a major rewrite, and greg has implemented the version as described above on that branch

@gbj
Copy link
Collaborator

gbj commented Jan 14, 2024

Yeah no worries at all. Your work here has been very helpful in figuring out the right way to do it. Luckily the new server fn setup lets us apply those ideas in an even simpler way. And I may even be able to get a form of the "success state encoded in URL" part going on that branch.

So, thanks for your work!

@benwis
Copy link
Contributor

benwis commented Jan 17, 2024

@SleeplessOne1917 We've put out a 0.6 beta release that should have these changes in it, if you'd like to try it and give feedback

@gbj gbj closed this Jan 20, 2024
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.

3 participants