Add dependency on lemmy-client #51
Conversation
|
i know this is WIP but just to save time, i disagree with everything you've said and done in this PR for reasons i have stated in other discussions we have all had together it's obvious we have a fundamental misunderstanding of how this stuff works. this is unfortunate to say the least, sorry, and re-writing the whole app in this manner is not something i can get behind focusing on solving #49 only would be more useful IMO |
|
What's negative about leptos-query, it seems to have a lot of benefits. |
|
@jim-taylor-business I've outlined the benefits of using leptos-query, along with the rest of my changes, above. IIRC your reasons for not using it was that it didn't work on your machine when you ran the app. I at least ask that you run this PR branch locally (along with a locally running lemmy backend with the latest changes on the main branch) to see if you still encounter errors, and reviewing the code to see if you agree with my comments about the readability benefits, before so strongly disagreeing with my approach. |
| } | ||
| "daisyui": "^4.7.3" | ||
| }, | ||
| "packageManager": "pnpm@8.15.4" |
There was a problem hiding this comment.
The latest version of pnpm adds this when I install or update deps for some reason.
| #[cfg(not(feature = "ssr"))] | ||
| { | ||
| let iw = window() | ||
| .inner_width() | ||
| .ok() | ||
| .map(|b| b.as_f64().unwrap_or(0.0)) | ||
| .unwrap_or(0.0); | ||
|
|
||
| let on_resize = move |_| { | ||
| let iw = window() | ||
| .inner_width() | ||
| .ok() | ||
| .map(|b| b.as_f64().unwrap_or(0.0)) | ||
| .unwrap_or(0.0); | ||
|
|
||
| let mut query_params = query.get(); | ||
| if iw >= 2560f64 { | ||
| query_params.insert("limit".into(), "30".to_string()); | ||
| } else if iw >= 1536f64 { | ||
| query_params.insert("limit".into(), "20".to_string()); | ||
| } else { | ||
| query_params.remove("limit"); | ||
| } | ||
|
|
||
| if iw >= 640f64 { | ||
| csr_infinite_scroll_posts.set(None); | ||
| csr_paginator.set(None); | ||
| } | ||
|
|
||
| let navigate = leptos_router::use_navigate(); | ||
| navigate( | ||
| &format!("{}", query_params.to_query_string()), | ||
| Default::default(), | ||
| ); | ||
| }; | ||
|
|
||
| window_event_listener_untyped("resize", on_resize); | ||
|
|
||
| if let Ok(e) = web_sys::Event::new("resize") { | ||
| on_resize(e); | ||
| } | ||
|
|
||
| if iw < 640f64 { | ||
| let on_scroll = move |_| { | ||
| let h = window() | ||
| .inner_height() | ||
| .ok() | ||
| .map(|b| b.as_f64().unwrap_or(0.0)) | ||
| .unwrap_or(0.0); | ||
| let o = window().page_y_offset().ok().unwrap_or(0.0); | ||
| let b = f64::from(document().body().map(|b| b.offset_height()).unwrap_or(1)); | ||
|
|
||
| let endOfPage = h + o >= b; | ||
|
|
||
| if endOfPage { | ||
| create_local_resource( | ||
| move || (user.get(), list_func(), sort_func()), | ||
| move |(_user, list_type, sort_type)| async move { | ||
| let form = GetPosts { | ||
| type_: list_type, | ||
| sort: sort_type, | ||
| community_name: None, | ||
| community_id: None, | ||
| page: None, | ||
| limit: None, | ||
| saved_only: None, | ||
| disliked_only: None, | ||
| liked_only: None, | ||
| page_cursor: csr_paginator.get(), | ||
| show_hidden: None, | ||
| }; | ||
|
|
||
| let result = LemmyClient.list_posts(form).await; | ||
|
|
||
| match result { | ||
| Ok(mut o) => { | ||
| csr_paginator.set(o.next_page); | ||
| let mut p = csr_infinite_scroll_posts.get().unwrap_or(vec![]); | ||
| p.append(&mut o.posts); | ||
| csr_infinite_scroll_posts.set(Some(p)); | ||
| } | ||
| Err(e) => { | ||
| error.set(Some(e)); | ||
| } | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| window_event_listener_untyped("scroll", on_scroll); | ||
| } |
There was a problem hiding this comment.
I don't understand why we were using different fetch limits for different screen sizes. As for the infinite scroll logic, there is a library with various leptos utility functions that would greatly simplify things.
There was a problem hiding this comment.
more screen space, more content
it does work without it. "seems" is never good enough when you're actually going to do something, |
|
i've said all this before but here's my disagreements with this again:
i hope things are more stable now also, these extra libraries caused show stopping issues in the past |
I think I'm starting to get a better understanding of the issue you have with my approach. If I understand you correctly, regardless of whether or not the caching and invalidation functionality provided by lemmy-client works/is useful, we haven't gotten to a point where those are issues yet and my usage of the library is a premature optimization. Is that an accurate interpretation?
Looking back at the PR you introduced that change in, the problem you were solving was ds9.lemmy.ml not sending back cache control headers. This isn't a problem when testing against a locally running instance. @dessalines is the the lack of cache control headers in ds9's responses a deliberate configuration choice or accidental misconfiguration?
Makes sense considering cookies were also being used for language and theme. I've detailed my thoughts on using cookies for those in the OP. I think the
|
yes, it's a solution looking for a problem at the moment. it has caused all sorts of problems for me in the past. i'm guessing it will not solve the caching problem. i haven't seen it progressively enhance to CSR only mode. so at the moment it obfuscates server communication and the vanilla leptos version works fine without it.
i have replicated this locally. i totally agree that if we are going to use the
i agree the final solution has not been defined yet and will involve the lemmy server significantly so what is there is just useful for testing and i think, for now, is a nice feature. i also don't think we need to copy what's currently happening in Lemmy UI if it is small useful addition to a feature.
CSR mode is naturally used when the same code is used for a Android/iOS/Windows/MacOS app and it is currently a declared objective of the project on the homepage so that's why i made everything work with that mode. it's a feature of Leptos generally and i think the whole isomorphic rust thing is cool.
cool. i seem to remember i put the site state in the context as well and had some performance issues with it as it can be pretty large and is copied around a lot which is why i went for a simple boolean flag to indicate logged in status. that was a while ago though so Leptos might have optimized that better now. oh actually now i think about it contexts don't work in SSR mode so i had to make the site info a signal passed down in a property. |
Those are set in the back-end, and don't look like they're being overridden in nginx, but I do see that ds9.lemmy.ml is running a test back-end version. voyager.lemmy.ml is on |
the reason the the browser cache defaults to "private" when you're logged in. i guess because there is a auth token header set at that time. when you are logged out the cache default is 60s which is why the bug occurs when you immediately log back in. the work around is not to refresh the another work around is to explicitly prevent caching by setting headers in lemmy server on the both work in my testing |
|
@jim-taylor-business now that I better understand your gripe with leptos-query, I think we're on the same page. At this point, I could rewrite what I currently have to use vanilla resources, though I want to hold off pulling the trigger unless smoke testing of the app reveals that it's causing problems. Regarding the caching issues you mentioned: is there a way to consistently reproduce the bug? It could very well be a browser issue as you said. I've tested my changes on Firefox, Chromium, and Librewolf and haven't run into the issue. About CSR: I haven't tested that yet and definitely need to make sure my changes work there (which I doubt they do at the time of making this comment). Do you use trunk to test CSR? Or is there some feature of cargo leptos that lets me use CSR instead of SSR with hydrate? Finally, I will re-add the cookie stuff for the languages and themes, although I would like to keep the actix session approach for handling the auth cookie. |
so is the only reason these two libraries are being used is that you are convinced they will be useful in the future?
the cache bug details are in my post just above and are reproduced in the end to end tests. maybe something you've already done has fixed it.
yeah you can use |
|
I removed leptos-query from the dependencies for now. I thought I needed it to manually refetch the resources, but it turns out resources already have that functionality. I'll see what I can do about removing actix-session. I will be shifting my priority to the JS UI for the next few days to get it ready for 0.19.4. |
|
cool, good luck with Lemmy UI i'm just interested to know what your idea is for the design in future and how these crates play a role, because i do recognise that they do things that could be useful and i just can't think of the features or design where that happens |
|
No probs, and no rush on this of course, but it still would be nice to have this dependent on your lemmy-client-rs, I think we're all in agreement there. |
|
@jim-taylor-business How did you run the project with CSR before? Trunk's docs mention that it needs to have an |
|
ping me when you want me to take another look. |
|
@dessalines trying to make this work in CSR while using the server actions/functions as a single source of truth for making API requests lead me down a rabbit hole. You can see an experiment I made investigating this here: https://github.com/SleeplessOne1917/trunk-cargo-leptos-example. If you're fine needing to go through some inconvenient steps to test this locally, I think this is good to take out of draft. |
| leptos = { version = "0.6", features = ["nightly"] } | ||
| leptos_meta = { version = "0.6", features = ["nightly"] } | ||
| leptos_router = { version = "0.6", features = ["nightly"] } |
There was a problem hiding this comment.
Leptos 0.7 is going to be a full rewrite of the reactive system. While the maintainer said he is doing everything he can to keep the API changes minimal, I think it's safer to handle upgrading to 0.7 as its own PR. At the very least, I know there are changes in how suspenses are handled in 0.7. You can learn more about the changes coming in 0.7 in the most recent monthly meetup.
I actually tested it as part of a Tauri app and generated some HTML and CSS in that project. I found that leptos handles the transition to CSR-only very gracefully as long as you don't force the use of the leptos server functions |
|
@SleeplessOne1917 is there any chance you could make a new summary of what's changed just to provide an overview? I'm guessing the summary at the beginning of this PR is out of date. just a bullet list of the improvements and feature removals or changes would give a better idea of what you've done and why |
|
@SleeplessOne1917 i can't see any handling of the errors returned by the lemmy_client or any user feedback also the login goes into some kind of infinite loop on a bad login. i guess this is still WIP as i notice the pipeline tests are not passing? |
@SleeplessOne1917 in CSR mode the Leptos SSR server does not exist. Trunk is only used to test this because it is a simple web server. when you are running the code on the desktop or mobile app Trunk is not guaranteed to exist either. the HTML and CSS will be hard coded and everything will be rendered in the browser and the only server to be communicated with will be the Lemmy instance. that's why the client moves to using the direct communication to the lemmy instance, it doesn't need the server function as a proxy. you can easily share common functions (sources of truth) between the two. |
That explains why I couldn't get it working with Trunk out of the gate.
I'll look over the changes again and make a review either later tonight or tomorrow. The scope of this PR definitely crept while I was working on it.
I admittedly went down a rabbit hole trying to figure out how to reuse server actions between all of our use cases to no avail. I'll shift closer to what you were originally doing for CSR, although I'm going to try using client side actions before trying regular on submit handlers. Also, are we set on Tauri as the desktop app framework? I know in the README it says we've yet to agree on a framework for that, but it's the only framework I know of that lets us use our webapp code for a desktop app. The closest thing to alternative that meets that criterion and comes to my mind is if we wait for if/when Leptos supports being use with GTK or another UI library, but the closest thing that exists to that currently is a tiny demo example app. If we are going with Tauri, we may have a better way of storing sessions/selected theme than using cookies since we'll have access to more than just the browser APIs. |
|
Per @jim-taylor-business's request, here is a list of changes. Additions
Removals
Regarding CSR/Desktop AppAfter working and thinking on this awhile, I'm starting to question if we should be worrying about CSR at this point of development. We know for certain that this is going to be used in the browser, so handling SSR and hydrated cases is a given. However, how strongly decided are we on making this into a desktop app as well? I ask because having access to more than just the browser APIs on the frontend likely warrants doing some things differently. For example, it could be the case that something like tauri-plugin-stronghold is a more appropriate way of storing login sessions than cookies. I consulted both the Leptos and Tauri discords on the subject and there is no preferred/idiomatic way of doing this. It doesn't help that tauri + leptos examples and templates are few and far between, and none I found demonstrated managing sessions. Since this is already a big initiative with it being a re-implementation of a large existing frontend we want to reach feature parity with, and since how to handle a leptos app with tauri is currently sparse on examples we can reference and completely undocumented, I think it would be wiser to focus on making this a working webapp for use in browsers (so SSR and hydrate) first and refactor if we ever end up using this for a desktop app as well. |
|
I've had no time to work on this tho, so I really don't get a say. IMO we should prioritize this as an SSR app. This is still a web framework, not "native" at all. If someone wants to create a native app they should use a desktop UI framework like GTK or QT or one of the rust cross-platform-ish ones, or else just create a thin web wrapper that calls the SSR. A javascript SPA (which is what the CSR would be), doesn't really make sense IMO. |
Closes #49.
This is a big one, so I'll give some more info so this is more digestible.
Resource handling
I went back to the
leptos-queryapproach of resource handling. To reiterate what I said in a comment in the last big PR:In addition to above, I also found the code to be shorter and easier to read after using this approach in combination with some other things I will get to shortly.
Cookies
I'm using
actix-sessionagain for this. Besides (in my opinion) being easier to work with than the existing approach, it allows us to always useHttpOnlyin the session cookie, making it more secure as it mitigates common XSS attacks. There were 2 other things that used cookies that I removed/commented out that I must also touch on:Language
I don't think it makes sense to use a cookie for this. This is the sort of thing that can be derived, in order of precedence, from the user language, site language, and browser language. A cookie doesn't solve any problems here.
Theme
This is a more interesting issue. I would normally prefer to use a cookie for the theme, but the fact that theme is also tracked as a user setting and site setting in the backend makes me wonder if we should use an approach more similar to the existing inferno UI. This concern would be easier to disregard if this was a third-party UI, but the fact that this is meant to replace the default/official UI makes me think we should honor that.
This conversation can be extended to many of the other display related settings as well, e.g. post listing types (maybe a frontend wants to have its own post listing types, or maybe a user would prefer card view on their mobile app and list view on their browser).
Errors
Had to comment them out because they conflicted with what I was implementing. Will have to revisit them before this PR is taken out of draft.
Unpack
This pairs quite nicely with leptos query. It allows use to use the item from a query - or signals derived from one - to work cleanly and concisely with suspenses and error boundaries.
Misc.
Layoutand moved markup to surroundRoutescomponent instead since it wraps all the routes anywayhash-filescargo leptos option, which adds a hash to the CSS, JS, and WASM files that get served between each build