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

feat: generalize response data injection #3188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CertainLach
Copy link

I have a leptos styled components library, which is using leptos_meta for SSR for a long time.

However, I had some problems with this flow, that are probably solvable by extension of leptos_meta public API, but I decided to generalize leptos_meta server integration instead, now any other library may hook into response data generation.

This PR only implements axum integration, because I'm not sure if there is a better way to implement this feature.

@gbj
Copy link
Collaborator

gbj commented Nov 3, 2024

Can you say more about the problem you're trying to solve, here? There are some very significant drawbacks to making a breaking change to every application simply to add more verbosity for 99% of users, so I am wondering if there is another way to solve this than adding the generics you've added.

@CertainLach
Copy link
Author

CertainLach commented Nov 3, 2024

Basically, I want to inject a single <style> (extracted critical CSS) block to head, where contents of this style block are being populated on the fly, by include_sass! macro.
Currently, without this patch, code looks like the following:

#[component]
fn PermissionConfirmation(
        callback: ChallengeCallback,
        app_name: String,
        new_permissions: Vec<DisplayPermission>,
        kept_permissions: Vec<DisplayPermission>,
) -> impl IntoView {
        include_sass!(css_module, "permissions.scss");
        let callback = {
                move |_: MouseEvent| {
                        callback.run((
                                control::serialize(&PermissionsConfirmationResponse { confirmed: true }),
                                None,
                        ))
                }
        };
        h!(div[[
                CssModule > { css_module },
                h1 > { app_name },
                table.permissions[[for permission in new_permissions {
                        tr.permission[[
                                td.icon[[img(src = permission.icon.clone())]],
                                td.name[[{ permission.name.clone() }]],
                                td.description[[{ permission.description.clone() }]],
                        ]]
                }]],
                table.permissions[[for permission in kept_permissions {
                        tr.permission[[
                                td.icon[[img(src = permission.icon.clone())]],
                                td.name[[{ permission.name.clone() }]],
                                td.description[[{ permission.description.clone() }]],
                        ]]
                }]],
                ul.login_btn[[
                        li[[button(onclick = callback) > "Confirm"]],
                        li[[button > "Cancel (Or close page)"]],
                ]],
        ]])
}

(Don't mind the custom view macro, it is required for the proper css module scoping)
include_sass macro generates a bunch of

let cl_login_btn = "mangled-class-name";
...
let css_module = DataForCssModule {...};

local variables, and the custom view macro uses them

But it also requires an additional CssModule component present, which handles the lifecycle of <Style> component children. (Which is pretty complicated, because if the same style file is imported multiple times - only one Style component should be present)

I have tried to reimplement this with direct usage of MetaContext/ServerMetaContext inside of the hook, but the lifecycle is still not great, and there is no way with the current ServerMetaContext api to be able to merge multiple added style tags into one.

// expansion of use_sass!
let cl_login_btn = "mangled-class-name";
...
use_css_module(DataForCssModule {...});

#[component]
fn PermissionConfirmation(
        callback: ChallengeCallback,
        app_name: String,
        new_permissions: Vec<DisplayPermission>,
        kept_permissions: Vec<DisplayPermission>,
) -> impl IntoView {
        include_sass!("permissions.scss");
        ...
        h!(div[[
                ...
                ul.login_btn[[
                        li[[button(onclick = callback) > "Confirm"]],
                        li[[button > "Cancel (Or close page)"]],
                ]],
        ]])
}

To bypass the limitations of ServerMetaContext api, without the need to implement css modules building blocks in leptos itself, I have decided to implement my own response head injector, like leptos_meta does. And to do that - I need some way to make axum integration to use this response injector, where it already has hardcoded integration for leptos_meta.

I agree that the current approach with ServerExtension generic parameter is not great, but I haven't found a prettier way with the current leptos SSR API. I will re-implement this PR, if someone proposes a better API.
...It is also possible to push this breaking change with 0.7.0 major release, as it also gives some freedom to application shell construction, which gives false sense that leptos_meta is not something tightly coupled with core, and something users can implement themselves.

@gbj
Copy link
Collaborator

gbj commented Nov 4, 2024

false sense that leptos_meta is not something tightly coupled with core, and something users can implement themselves.

The problem here is not that leptos_meta is tightly coupled to the core, and that users can't implement itself; the problem is that leptos_axum is designed as a convenient wrapper around the combination of leptos_router, leptos_meta, and leptos. It's just existed as a convenience for so long that it seems like a necessity; but it's really not.

It is also possible to push this breaking change with 0.7.0 major release

It's definitely possible. When I call it a "breaking change" I don't mean "it's a breaking change according to semver" I mean "it breaks users' applications" — see the fact that every example fails CI without modification. I do not plan to make this change, whether it is allowed by semver or not.

there is no way with the current ServerMetaContext api to be able to merge multiple added style tags into one.

Fair enough! Is there a way we can adjust the leptos_meta API to support it what you need? It might be useful to compare to the discussion in #2856, too, which I think had similar interests.

What additional functionality do you need to support what you're trying to do? Are there one or two functions that could be added to enable it?

@CertainLach
Copy link
Author

CertainLach commented Nov 4, 2024

It's just existed as a convenience for so long that it seems like a necessity; but it's really not.

The problem is, it is not easy to implement SSR without those helpers, => it is not easy to implement functionality like leptos_meta by yourself.

see the fact that every example fails CI without modification

I agree that the current approach is bad, but it might be improved.

  • Extension parameter may have a default type (Error-prone, as if user wants to specify it - it needs to be specified everywhere)
  • Extension might be made a value, instead of being purely type-based. This still require modifications to user code (It might be moved to LeptosOptions, made an optional provider function like additional_context, or redone in other, more convinient way), but also will allow users to provide some values from outside (generalized ResponseOptions?)
  • Server functions might be wrapped into a some struct. This way you can even get rid of many generics everywhere (I.e server view definition can be done in a single place), but it requires users to update their codebases too.

What additional functionality do you need to support what you're trying to do? Are there one or two functions that could be added to enable it?

It might be solved by adding a couple of functions in public api, but there is no way to make this api look good, and be usable for everyone. My implementation only needs to

  • Create a style tag from inside of a hook. This is already possible by exposing register() function, sharing information about this style tag might be done by using user-provided context. Style tag needs not to be shared between library users (i.e here needs to be an ability to register multiple style tags), as e.g my css modules implementation takes control of this tag during hydration
  • Append contents to this style tag. Current implementation needs to be extended to not only be able to send new tags to ServerMetaContextOutput, but also to be able to update (append-only?) already sent tags. Creating multiple style tags is not an option, because for atomic css modules actual css content is too small, everything will be bloated by just the style tags, and it might be much harder to have flicker-free hydration in this case (during hydration, text-based style tag needs to be cleared for performance reasons, and then managed by CssStyleSheet api, I borrowed my implementation details from https://github.com/styled-components/styled-components)
  • Have some way to provide information from use-hooks to hydration stage. An additional modifiable <script> tag api? Some generic way of providing JSON value in SSR, e.g use_head().set_kv("a", "b") in SSR/use_head().get_kv("a") in hydration? Just an ability to set style tag attributes?
  • Have an ability to compute and update <style> nonce attribute for CSP purposes? Leptos can do it by itself, if this api is being specialized for style tags, and not just being exposed register() + append_to_registered() functions I was thinking about <script> subresource integrity attribute, nothing needs to be computed for <style> nonces, users can just read them from Nonce context and append to <style> on registration.

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.

2 participants