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

"It just works" dotnet-based SSR for Feliz #262

Open
l3m opened this issue Oct 6, 2020 · 7 comments
Open

"It just works" dotnet-based SSR for Feliz #262

l3m opened this issue Oct 6, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@l3m
Copy link
Contributor

l3m commented Oct 6, 2020

The thing I miss the most since switching from Fable.React is that even with Feliz/Feliz.ViewEngine, SSR is painful. It can work, but requires care and ..well, work, and makes code ugly.

In Fable.React, I was able to simply call a function and have all my stuff SSR'd. "It just works". And not only for SSR, it also helped me test much better and find some weird behaviors.

I would greatly appreciate to have that experience in Feliz, too. I feel that the complexity and ugliness of #if FABLE_COMPILER in every file (or using Isomorphic, which has yet another namespace and another set of issues) should be hidden in the library. I should be able to write

open Feliz 

and have it work in AspNetCore too.

I believe that especially in conjunction with Fable.Remoting, out of the box support for SSR would improve the value of a Feliz/AspNetCore stack and in general help the Full Stack F# story greatly.

What do you think?

@Shmew
Copy link
Collaborator

Shmew commented Oct 6, 2020

Yeah I'm not against that idea, it also keeps things synchronized.

@Zaid-Ajaj
Copy link
Owner

Hi @l3m, I really like the idea and of course having things "just work" would be ideal for users. However, this will immensely complicate the implementation details of the library and shift focus. From day of building this library, I deliberately left SSR support because of the messy details I had seen within Fable.React.

The best compromise right now is to use Fable.React for the SSR parts and Feliz for the rest or keep using Feliz/Feliz.ViewEngine.

The plans right now for Feliz is still to focus on pure React support and its ecosystem: proper Svg support #251, proper fast-refresh support (#157 and #260) with Fable 3 and implementing more full components with all the goodies (docs, examples etc)

@Zaid-Ajaj Zaid-Ajaj added the enhancement New feature or request label Oct 9, 2020
@l3m
Copy link
Contributor Author

l3m commented Oct 9, 2020

@Zaid-Ajaj Hm, that's an understandable reason, however, the issue I see with this approach is that all dependent libraries will also not implement SSR. If it would be in Feliz, all dependent libraries would have it too.

Which basically would mean: Feliz could be used with SSR for non-toy projects (where SSR is actually needed) - as using pure Feliz with no additional library support seems a rarer case for larger projects.

I'm asking because for us, SSR with Feliz worked until we had to use additional dependencies... and it seems hard to convince library authors to do the work if Feliz doesn't provide for it.

@Shmew
Copy link
Collaborator

Shmew commented Oct 10, 2020

Yeah that's a pretty convincing argument imo. I think we can reduce the clutter a fair bit due to how the library is already structured. All (or the vast majority) of the elements for example all call the same 2-3 functions, same with most of the properties and such. So I believe for the most part it would just require modifications at those sources. There's some special stuff done in view engine that I'm not really familiar with, perhaps @dbrattli can chime in on those/this topic.

@dbrattli
Copy link
Contributor

dbrattli commented Oct 10, 2020

Yes, I've been feeling the pain myself 😞. Mostly because of the different namespaces resulting in a lot of #if FABLE_COMPILER throughout the files. But also that Ionide chokes on ReactElement of Feliz.ViewEngine not being the same as ReactElement of Fable.React. Porting all libraries from Feliz e.g Feliz.MaterialUI to .ViewEngine and keep them in sync will be painful. I guess I have already given up keeping Feliz.Bulma.ViewEngine up-to-date.

The main problem with using Feliz code server side is that it uses a lot of e.g unbox<string>(value) instead of string value. What is the perf gain here? Could we use an inline function to hide the difference? There's also some Fable.Core.JsInterop stuff e.g: !!e?value <> !!value then e?value that needs to be handled differently server side.

Feliz.ViewEngine also uses it's own data model since parsing ReactElement back to HTML is not that easy. Thus it would be nice if we could generate the ViewEngine types in Interop.fs when !FABLE_COMPILER. ViewEngine types can still inherit from ReactElement so the signatures stays the same.

So I think it's feasible, but it will require a few changes and some #if FABLE_COMPILER to Feliz itself. But I would not start on such an effort until @Zaid-Ajaj gives an approved-in-principle to the idea of merging the libraries..

Note that similar changes needs to be done for e.g Feliz.Bulma that e.g also uses unbox<string>(value), but if we could have a common recommendation on how to do things so dependent libraries will eventually also work.

@Shmew
Copy link
Collaborator

Shmew commented Oct 10, 2020

To see why we use unbox<string> when possible consider this:

box/unbox is erased at compile time since it's JS.

Doing string i compiles into this:

// fable-library

export function int32ToString(i: number, radix?: number) {
  i = i < 0 && radix != null && radix !== 10 ? 0xFFFFFFFF + i + 1 : i;
  return i.toString(radix);
}

// Calling site
import { int32ToString } from "fable-library/Util.js";

export const test = int32ToString(1);

Doing unbox<string> 1 compiles into this:

export const test = 1;

So if you have 10 properties that all use restrictions on the input but the actual representation is a string that property will get recalculated every rerender of that component. The execution cost of a single instance of this is incredibly small, but when you can have anywhere from 1-20 of these on each component spanning across potentially hundreds of components reducing computation at these call sites is crucial.

Now, the solution for this is actually very simple:

let inline unboxStr o = 
	#if FABLE_COMPILER	
	unbox<string> o
	#else
	string o
	#endif

Then we just do a find replace on unbox<string>.

@Shmew
Copy link
Collaborator

Shmew commented Oct 10, 2020

Since most likely Fable 3 will involve a significant re-write, I think that's an opportune time to do this work while we fix the other issues @Zaid-Ajaj mentioned above. We're going to want to do this (I think it's very safe to say @Zaid-Ajaj is not against SSR, as much as against having mom's spaghetti spilled all over his code base), and I believe it will be the least painful/messy if it's done with SSR in mind from the get-go during that period.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants