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

Keep AddAnyAttr logic contained #3562

Merged
merged 12 commits into from
Feb 10, 2025
Merged

Keep AddAnyAttr logic contained #3562

merged 12 commits into from
Feb 10, 2025

Conversation

gbj
Copy link
Collaborator

@gbj gbj commented Feb 7, 2025

@zakstucke Could you help test this to see if it resolves the compile-time issues in the same way that #3553 does?

I think the idea of storing extra_attrs on the AnyView itself is reasonable. This approach removes the changes to the Render/RenderHtml traits and concentrates all those changes into AnyView itself. But I'm hopeful that it has the same benefit by not creating additional types for the AddAnyAttr implementation.

There is still some significant binary-size overhead that gets connected to every AnyView as a result (albeit slightly less). But it has the benefit of only touching this single file, without spreading through all the rest of the API, and it might also be possible to merge some of the separate erase-components cfg-gated logic back in.

If it doesn't fix the problem of course then the point is moot.

@zakstucke
Copy link
Contributor

zakstucke commented Feb 7, 2025

Hey @gbj, just tested this and no good, the fact you allow the next level of types to be made is is enough to reach the limit. This is why I'm guessing the lack of the feature is why stable leptos is actually compiling, because all Children, ChooseView etc types (that use AnyView) are currently barriers between this type explosion, i don't think the issue has anything specifically to do with AnyView per se.

I've just been using thaw to test, you can just check it out, point the cargo.toml to the leptos local path, then to repro it's literally just cargo build --features ssr --bin demo to see the blowup.

10 mins compiling, 50GB RAM, then error: cannot encode offset of relocations; object file too large

I'm going to be quite shocked after all the testing i've done if you find a solution that doesn't involve reducing the number of types somehow, or at the very least removing a bunch of generics (const bools, associated types etc) that helps reduce monomorphisation by n-fold.

@gbj
Copy link
Collaborator Author

gbj commented Feb 8, 2025

Huh. I am having trouble figuring out how to replicate this result, which makes it pretty hard for me to try out different solutions.

If I clone thaw, set a local leptos dependency, and go to demo and cargo build --features=ssr --timings:
- main branch: dies with error: queries overflow the depth limit!
- leptos_0.8 branch: dies with same error
- this PR: dies with same error

If I run RUSTFLAGS="--cfg erase_components" cargo build --timings --features=ssr:
- main branch: builds in 18s
- leptos_0.8 branch: builds in 16s
- this branch: builds in 17s

I may be doing something wrong.

EDIT: Ah okay so you're testing specifically without component erasure, right? Then this branch does seem to hang if I try to do, say trunk serve in the thaw demo. I haven't tested the current 0.8 branch because it requires updates to thaw to match the API changes in your PR, maybe I'll try that tomorrow.

I'm going to be quite shocked after all the testing i've done if you find a solution that doesn't involve reducing the number of types somehow

I'm sure you're right.

@zakstucke
Copy link
Contributor

zakstucke commented Feb 8, 2025

Yes my last PR is specifically trying to fix non-erased/normal/stable mode, with --cfg erase_components it will work everywhere because of #3518, which solves the issue by reducing the number of types through vectorisation (reducing tuple types).

The other thing to make sure is you're updating the leptos version when switching from 0.7 to 8, e.g. in 8 I have this in thaw's cargo.toml:

leptos = { version = "0.8.0-alpha", path = "../leptos/leptos" }
leptos_meta = { version = "0.8.0-alpha", path = "../leptos/meta" }
leptos_router = { version = "0.8.0-alpha", path = "../leptos/router" }
reactive_stores = { version = "0.1.3", path = "../leptos/reactive_stores" }

Behaviour you should be seeing:
cargo build --features ssr --bin demo:
0.7 main: works because AddAnyAttr is disabled (if you enable it it would hang)
0.8 branch: works because of the passthrough of attrs without producing a new layer of types in my last PR
this PR: hangs

RUSTFLAGS="--cfg erase_components" cargo build --features ssr --bin demo
All branches should work, thanks to #3518 that provides the necessary reduction in types to solve it that way. But this method isn't suitable for non-erased mode.

For 0.8 branch, here's the changes for thaw's `ssr_mount_style.rs`:

There's also one view.build() -> view.build(None) in thaw_components/src/teleport/mod.rs

use leptos::{
  attr::{any_attribute::AnyAttribute, Attribute},
  context::{Provider, ProviderProps},
  prelude::*,
  tachys::{
      hydration::Cursor,
      renderer::types,
      view::{any_view::{AnyViewState, ExtraAttrsMut}, Position, PositionState},
  },
};
use std::collections::HashMap;

#[component]
pub fn SSRMountStyleProvider<Chil>(children: TypedChildren<Chil>) -> impl IntoView
where
  Chil: IntoView + 'static,
{
  let context = SSRMountStyleContext::default();

  let children = Provider(
      ProviderProps::builder()
          .value(context.clone())
          .children(children)
          .build(),
  )
  .into_any();
  SSRMountStyle { context, children }
}

#[derive(Debug, Clone)]
pub struct SSRMountStyleContext {
  styles: ArcStoredValue<HashMap<String, String>>,
}

impl SSRMountStyleContext {
  pub fn use_context() -> Option<Self> {
      use_context()
  }

  pub fn push_style(&self, k: String, v: String) {
      self.styles.write_value().insert(k, v);
  }

  fn default() -> Self {
      Self {
          styles: Default::default(),
      }
  }

  fn html_len(&self) -> usize {
      const TEMPLATE_LEN: usize = r#"<style id=""></style>"#.len();
      let mut html_len = 0;
      let styles = self.styles.write_value();

      styles.iter().for_each(|(k, v)| {
          html_len += k.len() + v.len() + TEMPLATE_LEN;
      });

      html_len
  }

  fn to_html(self) -> String {
      let mut styles = self.styles.write_value();
      styles
          .drain()
          .into_iter()
          .map(|(k, v)| format!(r#"<style id="{k}">{v}</style>"#))
          .collect::<String>()
  }
}

pub struct SSRMountStyle {
  context: SSRMountStyleContext,
  children: AnyView,
}

pub struct SSRMountStyleState {
  state: AnyViewState,
}

impl Render for SSRMountStyle {
  type State = SSRMountStyleState;

  fn build(self, extra_attrs: Option<Vec<AnyAttribute>>) -> Self::State {
      let state = self.children.build(extra_attrs);
      SSRMountStyleState { state }
  }

  fn rebuild(self, state: &mut Self::State, extra_attrs: Option<Vec<AnyAttribute>>) {
      self.children.rebuild(&mut state.state, extra_attrs);
  }
}

impl AddAnyAttr for SSRMountStyle {
  type Output<SomeNewAttr: Attribute> = Self;

  fn add_any_attr<NewAttr: Attribute>(self, _attr: NewAttr) -> Self::Output<NewAttr>
  where
      Self::Output<NewAttr>: RenderHtml,
  {
      self
  }
}

impl RenderHtml for SSRMountStyle {
  type AsyncOutput = Self;
  type Owned = Self;

  const MIN_LENGTH: usize = 0;

  fn html_len(&self, extra_attrs: Option<Vec<&AnyAttribute>>) -> usize {
      self.children.html_len(extra_attrs) + self.context.html_len()
  }

  fn dry_resolve(&mut self, extra_attrs: ExtraAttrsMut<'_>) {
      self.children.dry_resolve(extra_attrs);
  }

  async fn resolve(self, _extra_attrs: ExtraAttrsMut<'_>) -> Self::AsyncOutput {
      self
  }

  fn to_html_with_buf(
      self,
      buf: &mut String,
      position: &mut Position,
      escape: bool,
      mark_branches: bool,
      extra_attrs: Option<Vec<AnyAttribute>>,
  ) {
      self.children
          .to_html_with_buf(buf, position, escape, mark_branches, extra_attrs);

      let head_loc = buf
          .find("<head>")
          .expect("you are using SSRMountStyleProvider without a <head> tag");
      let marker_loc = buf
          .find(r#"<meta name="thaw-ui-style""#)
          .unwrap_or(head_loc + 6);

      buf.insert_str(marker_loc, &self.context.to_html());
  }

  fn to_html_async_with_buf<const OUT_OF_ORDER: bool>(
      self,
      buf: &mut leptos::tachys::ssr::StreamBuilder,
      position: &mut Position,
      escape: bool,
      mark_branches: bool,
      extra_attrs: Option<Vec<AnyAttribute>>,
  ) where
      Self: Sized,
  {
      self.children
          .to_html_async_with_buf::<OUT_OF_ORDER>(buf, position, escape, mark_branches, extra_attrs);

      buf.with_buf(|buf| {
          let head_loc = buf
              .find("<head>")
              .expect("you are using SSRMountStyleProvider without a <head> tag");
          let marker_loc = buf
              .find(r#"<meta name="thaw-ui-style""#)
              .unwrap_or(head_loc + 6);
          buf.insert_str(marker_loc, &self.context.to_html());
      });
  }

  fn hydrate<const FROM_SERVER: bool>(
      self,
      cursor: &Cursor,
      position: &PositionState,
      extra_attrs: Option<Vec<AnyAttribute>>,
  ) -> Self::State {
      let state = self.children.hydrate::<FROM_SERVER>(cursor, position, extra_attrs);
      SSRMountStyleState { state }
  }

  fn into_owned(self) -> Self::Owned {
      self
  }
}

impl Mountable for SSRMountStyleState {
  fn unmount(&mut self) {
      self.state.unmount();
  }

  fn mount(&mut self, parent: &types::Element, marker: Option<&types::Node>) {
      self.state.mount(parent, marker);
  }

  fn insert_before_this(&self, child: &mut dyn Mountable) -> bool {
      self.state.insert_before_this(child)
  }
}

@gbj
Copy link
Collaborator Author

gbj commented Feb 8, 2025

Okay, I'm pretty confident I've got a handle on what does/doesn't work now. Thanks for your help as I slowly wrap my head around it 😄

I'm about halfway through an implementation that adds an elements() method to Mountable types, which can be used to build an AnyView with additional attributes without composing the inner type with Vec<AnyAttribute>, by building the AnyView, then getting a list of its elements, then applying each attribute to each element. I also split this apart from AnyView by adding an AnyViewWithAttrs that is created in AddAnyAttr for AnyView.

As a result, this

  • does not create another level of types inside AnyView, so it compiles the thaw demo etc.
  • adds less binary-size overhead for WASM (doesn't pass possible dynamic stuff everywhere, only if you actually use spreading and therefore create an AnyViewWithAttrs
  • will probably use the parts of your approach that thread additional attributes through SSR, where I'm not as concerned as binary size and where the elements()-based approach doesn't work

I'll just put WIP commits here as I know you are simultaneously working on other things that I see in your binary-size PRs.

@gbj
Copy link
Collaborator Author

gbj commented Feb 9, 2025

Okay well I guess I have to change the PR title 😅 because this isn't "contained" per se. However, it completely removes my binary-size concerns about #3553, while using the same approach.

@zakstucke I have tested this against the thaw example... if you could double-check for me by running against your own code (which I think was the original test case, right?) I'd appreciate it. I have tested this and the attribute spreading works out very well in SSR.

Thanks again for your work. I don't think I ever would have figured out how to do this without you.

@zakstucke
Copy link
Contributor

zakstucke commented Feb 9, 2025

@gbj This looks great! If you're confident that the html elements themselves are the only thing "needing to know" about attributes now or in the future, then this sounds like a great solution to get around needing to thread extra attrs through the system on the wasm side.

I've tested this and it seems to work!

Some extras:

  • Can we cfg flag out the dry_resolve/resolve/to_html_ trait methods when not ssr? I've got a feeling this might improve binary size if the compiler isn't optimising them out perfectly, or at the very least compile times.
  • Can we keep the into_owned method I added to RenderHtml? I added it to fix something separate to AddAnyAttr (it just ended up in that PR): when you switch a project to erased mode, there'll be a few compiler errors where impl IntoView types need to be marked as 'static, or you actually need to make them static downstream. By adding it we can make impl IntoAny for T not require T to be static anymore, getting around this issue. This becomes more important in some unfinished PR's I've got that make erased mode even faster to compile, but rely on this too. I can add it back in a separate PR after this get's merged.

Thanks so much for your work on this too 🙏

@zakstucke zakstucke mentioned this pull request Feb 9, 2025
@gbj
Copy link
Collaborator Author

gbj commented Feb 10, 2025

Okay, great.

Can we cfg flag out the dry_resolve/resolve/to_html_ trait methods when not ssr? I've got a feeling this might improve binary size if the compiler isn't optimising them out perfectly, or at the very least compile times.

Hm... I'm open to it if you find that it does reduce either binary size or compile time (for the end-user's crate -- I imagine it will marginally reduce compile times for tachys etc), although I'd be a little surprised if it did (but not a lot surprised) The main downside is that cfg-ing out methods can hurt the discoverability in docs, but we would just need to make sure docsrs is configured probably to build with all features and to flag the methods by trait. I do this for any_spawner now.

Can we keep the into_owned method I added to RenderHtml? ... I can add it back in a separate PR after this get's merged.

Yes! I had forgotten you added it/didn't realize this was what it was for (removing the need for extra 'static bounds) but I'm all for it. If you could add it back in a PR that would be great. I'll merge this one for now.

@gbj gbj merged commit 299acd2 into leptos_0.8 Feb 10, 2025
62 of 74 checks passed
@gbj gbj deleted the reduce-erasure branch February 10, 2025 01:46
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