Zaid-Ajaj / Feliz

A fresh retake of the React API in Fable and a collection of high-quality components to build React applications in F#, optimized for happiness
https://zaid-ajaj.github.io/Feliz/
MIT License
534 stars 78 forks source link

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

Open l3m opened 3 years ago

l3m commented 3 years ago

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 commented 3 years ago

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

Zaid-Ajaj commented 3 years ago

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)

l3m commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

Yes, I've been feeling the pain myself :disappointed:. 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 commented 3 years ago

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 commented 3 years ago

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.