magento-research / peregrine

(Defunct) Moved to https://github.com/magento-research/pwa-studio
Open Software License 3.0
29 stars 1 forks source link

Add <ClientSideOnly /> component #20

Closed DrewML closed 6 years ago

zetlen commented 6 years ago

@jimbo To your first question, I think Router has a different use case. We wouldn't want a component option, for instance, because in the typical use case, we wouldn't want the dev to feel like their "context" has changed. They should be able to pass props down, rather than pass a constructor.

To the second question--I don't know! Maybe React has to blow the whole UI away rather than build a reconciliation.

DrewML commented 6 years ago

If I recall correctly, when the tree rendered by the client doesn't exactly match the tree rendered by the server, React throws an error. Are we going to encounter that error when using this component?

That's....a very good point that I hadn't considered. Some relevant reading:

Abramov made the following comment that is worth calling out:

image

Thoughts?

zetlen commented 6 years ago

Thanks, Dan, that's a good idea. Seems like this requires passing a function-as-prop (or function-as-child?) which receives hasMounted as an argument...

jimbo commented 6 years ago

Abramov's example is correct, but isn't the best solution. I don't think local component state is the right place to maintain the isSSR flag that a component checks. Here's how that would look:

  1. user hard-navigates to /
  2. server renders Home.js (isMounted: false)
  3. client renders Home.js (isMounted: false)
  4. client re-renders Home.js (isMounted: true)
  5. user soft-navigates to /foo
  6. client renders Foo.js
  7. user soft-navigates back to /
  8. client renders Home.js (isMounted: false)
  9. client re-renders Home.js (isMounted: true)

Only the first render of a user's session is done on the server. Every subsequent render is done by the client—even visiting new routes—so we once we're client-side, we shouldn't use the double-render flow. If the isMounted: true state is local to a component, though, we lose that state when it unmounts.

Since the server's render is a one-time global event, we should probably store the flag in global state—in the Redux store—and change it from false to true in our ReactDOM.render() callback. Maybe this ClientSideOnly component can be connected to Redux and read that flag? Here's how that flow would look:

  1. user hard-navigates to /
  2. server renders Home.js (clientHasRendered: false)
  3. client renders Home.js (clientHasRendered: false)
  4. client re-renders Home.js (clientHasRendered: true)
  5. user soft-navigates to /foo
  6. client renders Foo.js
  7. user soft-navigates back to /
  8. client renders Home.js (clientHasRendered: true)

What do you guys think? @zetlen @DrewML

zetlen commented 6 years ago

@jimbo has a good point that local state seems wasteful for a one-time global event. I wonder, though, if this can be a compile-time optimization...

components/foo.js

/** global ClientOnly */
import React from 'react'
// note that ClientOnly is not imported; this way it's easier to detect at
// build time by string

export default class Foo extends React.Component {
    render() {
        return (<ClientOnly onServerRender={<div>Server content</div>}>
          <span>Client content</span>
        </ClientOnly>)
    }
}

in a webpack plugin

compiler.parser.plugin('<ClientOnly', expr => {
  /** here or at some other hook, detect its presence */
})

at server component build time

  1. remove the node
  2. inline the onServerRender prop body if it exists:

build/server/components/foo.js

/** global ClientOnly */
import React from 'react'

export default class Foo extends React.Component {
    render() {
        return <div>Server content</div>
    }
}

at client component build time

  1. insert a noop definition ClientOnly = p => p.children
  2. remove the onServerRender prop

in bundle:

import React from 'react'
const ClientOnly = p => p.children

export default class Foo extends React.Component {
    render() {
        return <ClientOnly><span>Client content</span></ClientOnly>
    }
}

Potentially you could even detect if ClientOnly has just one child node and remove it entirely, inlining its contents.

zetlen commented 6 years ago

@DrewML @jimbo bump

DrewML commented 6 years ago

I'd prefer @jimbo's approach. I'm not a big fan of moving things to build time optimizations unless it actually provides some sort of DX or performance improvements (doesn't seem like that will).

jimbo commented 6 years ago

@zetlen That would be viable, but I'm reluctant to add another magic global (similar to a JSX pragma). I would prefer to do something more straightforward.

JulienPradet commented 6 years ago

Hi! I may have a few feedbacks around this.

First about the current API, I feel like onServerRender feels like an event when it's actually just a different children. I feel like it's clearer when you strip the on and just say serverRender. But that's actually just a feeling.

However, the more broader thing is that this component focus on stripping things out of the server. We have a few use cases in our stack that only changes props, rather than the full implementation. For instance, we don't want to load the number of items in the cart on the Server side.

So we just strip the graphql component which is supposed to load {cart: {quantity: 5}} by a static prop {cart: {quantity: 0}}.

For this reason, I feel like a more broader component is more useful. Something along those lines :

<IsServerSide>
  {isServerSide => {
    const Fetcher = isServerSide ? GraphQlFetcher : StaticFetcher
    return <Fetcher>{data => <ViewComponent data={data} />}</Fetcher>
  }}
</IsServerSide>

Actually it feels a bit weirder with renderProps and it works better for us with HOCs since we use them a lot for the data fetching part.

branchServerClient(
  withProps(props => ({
    cart: {quantity: 0 }
  })),
  graphql(CartQtyQuery)
)(ViewComponent)

But hah! Just to say that it's a tiny bit restrictive to phrase it as a ClientSideOnly component.
And also, obviously our use case is still doable with your current implementation.

zetlen commented 6 years ago

Thanks, @JulienPradet ! The reason we have to use render props instead of direct children is that without a render lambda, React always renders elements bottom-to-top. Therefore, both server and client content would attempt to fully render as vdom before the <ClientOnly> logic ran. We don't want to evaluate the server-side branch of the logic on the client-side or vice versa, since it may depend on something absent in that environment. So we need to use a render function somewhere, whether as a prop or as a child.

JulienPradet commented 6 years ago

Thank you for you answer @zetlen :)

I did use renderProps in my example. Even though they are within the children prop, it's still a renderProp and React would evaluate only the return statement of this lambda. If I use what's in the tests, it would look like :

<IsServerSide>
    {isServerSide =>
        isServerSide ? <div>Server Content</div> : <span>Client Content</span>
    }
</IsServerSide>;

// or

<IsServerSide
    render={isServerSide =>
        isServerSide ? <div>Server Content</div> : <span>Client Content</span>
    }
/>;

In case you were talking about the HOC I've shown, indeed, it's slowing down the initialization of the app (not the subsequent renders).

About using the the server-side logic on the client side, as @jimbo stated previously, it's mandatory if you want to have a correct hydration phase on the client. In our experience, the only thing that's safe here is to use client side logic that's not available on the server (window, DOM, refs, etc.). You can't really use things specific to the server environment without breaking the hydration (except for caching strategies).

DrewML commented 6 years ago

Too many concurrent things going on with higher priority, so haven't been able to give this much more attn.

Will revisit with further discussions about community contribution opportunities for optional SSR later on.