ctrlplusb / react-async-component

Resolve components asynchronously, with support for code splitting and advanced server side rendering use cases.
MIT License
1.45k stars 62 forks source link

[Discussion] Advanced use case #1

Closed bradennapier closed 7 years ago

bradennapier commented 7 years ago

Trying to convert react-universally to use this now while I build up the routes management structure and having issues.. .my app is mounting twice, it runs completely and renders then withAsyncComponents forces it to launch again and it causes all kinds of havoc.

const RenderedApp = ({ App, store }) => (
  <ReduxProvider store={store}>
    <BrowserRouter>
      <App store={store} />
    </BrowserRouter>
  </ReduxProvider>
)

function renderApp(store) {
  /* Get our Rendered App & Redux Store */
  // πŸ‘‡ run helper on your app and get back a result object.
  //    ❗️ The result includes a decorated version of your app
  //    that will allow your application to use async components
  //    in an efficient manner.
  withAsyncComponents(<RenderedApp App={App} store={store} />).then(
    ({ appWithAsyncComponents }) => {
      console.log('With Async', appWithAsyncComponents)
      render(appWithAsyncComponents, container)
    }
  )
}

image

As you can see it calls the With Async log which renders our app however it has already finished rendering the app in the first place when this is called.

bradennapier commented 7 years ago

It does appear to be a problem that occurs the second HMR connects

ctrlplusb commented 7 years ago

Hmmm, I have it working perfectly in a client implementation. I will try get it integrated into the next branch or as a PR on react-universally as soon as I can.

bradennapier commented 7 years ago

It would appear I should be expecting the app to have to render twice, right? walk tree seems to call and run all the components as it does it.

bradennapier commented 7 years ago

I figured out those errors were due to the app being rendered before the async call is made. I fixed the situation since my code didn't expect that and now it appears to startup but not sure if that caused any other issues. I see this in react-tree-walker:

// Call componentWillMount if it exists.
      if (instance.componentWillMount) {
        instance.componentWillMount();
      }
bradennapier commented 7 years ago

Just figured I would provide what I am doing in full for better context and since you will recognize the boilerplate and know where I did everything:

In my server render:

// server/middleware/reactApplication/index.js
const app = (
    <ServerRouter location={request.url} context={reactRouterContext}>
      <Provider store={store}>
        <App store={store} />
      </Provider>
    </ServerRouter>
  );

  // run helper on our app
  withAsyncComponents(app)
    .then(({ appWithAsyncComponents, state, STATE_IDENTIFIER }) => {
      // We need to renderToString
      const reactAppString = renderToString(appWithAsyncComponents);

      // Generate the html response.
      const html = generateHTML({
        // Provide the rendered React applicaiton string.
        reactAppString,
        // Nonce which allows us to safely declare inline scripts.
        nonce,
        // Running this gets all the helmet properties (e.g. headers/scripts/title etc)
        // that need to be included within our html.  It's based on the rendered app.
        // @see https://github.com/nfl/react-helmet
        helmet: Helmet.rewind(),
        // We provide our code split state so that it can be included within the
        // html, and then the client bundle can use this data to know which chunks/
        // modules need to be rehydrated prior to the application being rendered.
        // ~~ DISABLED IN FAVOR OF ASYNC COMPONENTS MODULE
        // codeSplitState: codeSplitContext.getState(),
        // Provide the redux store state, this will be bound to the window.__APP_STATE__
        // so that we can rehydrate the state on the client.
        initialState: getState(),
        asyncComponents: {
          state,
          STATE_IDENTIFIER,
        }
        // Pass through the react-jobs provided state so that it can be serialized
        // into the HTML and then the browser can use the data to rehydrate the
        // application appropriately.
        // Disabling ReactJobs for now during setup
        // jobsState: {
        //   state,
        //   STATE_IDENTIFIER,
        // },
      });
// generateHTML.js
...
${
          // Bind our async components state so the client knows about them
          asyncComponents
            ? inlineScript(`window.${asyncComponents.STATE_IDENTIFIER}=${serialize(asyncComponents.state)};`)
            : ''
        }
...

You have seen the client render method as originally posted...

Now I am trying to build a setup to automatically code split and render routes based on directory structure I use. I am fairly certain this should be setup properly.

// shared/app/routes.js

export default [
  {
    id: 'Home',
    exactly: true,
    props: {
      title: 'Home'
    },
    pattern: '/',
    component: () => import('./screens/Home')
  },
  {
    id: 'SecurityCenter',
    props: {
      title: 'Security Center'
    },
    pattern: '/security-center',
    component: () => import('./screens/SecurityCenter')
  }
]

Where I will pass the "component" prop to createAsyncComponent

// shared/app/App (I believe in original boilerplate it was just index.js here
render() {
    return (
      <div>
        {/*
          All of the following will be injected into our page header.
          @see https://github.com/nfl/react-helmet
        */}
        <Helmet
          htmlAttributes={safeConfigGet(['htmlPage', 'htmlAttributes'])}
          titleTemplate={safeConfigGet(['htmlPage', 'titleTemplate'])}
          defaultTitle={safeConfigGet(['htmlPage', 'defaultTitle'])}
          meta={safeConfigGet(['htmlPage', 'meta'])}
          link={safeConfigGet(['htmlPage', 'links'])}
          script={safeConfigGet(['htmlPage', 'scripts'])}
        />
        <Layers />
        <Navbar />
        {
          routes.map( route => {
            const matchProps = {
              ...route,
              key: route.id || 'Unknown',
              exactly: route.exactly == true,
              pattern: route.pattern,
            }
            return <Match {...matchProps} render={ renderProps => {
                const appProps = {
                  ...matchProps,
                  ...renderProps,
                }
                return (
                  <Body key={matchProps.key} {...matchProps.props}>
                    <AppMiddleware {...appProps} />
                  </Body>
                )
              }} 
            />
          })
        }
        <Miss component={Error404} />
        <LayerStackMountPoint />
      </div>
    )
  }

AppMiddleware is just a middle-man to connect to redux and render the async component

class AppMiddleware extends Component {

  shouldComponentUpdate(np) {
    if ( np.location.pathname !== this.props.location.pathname ) {
      return true
    }
    console.info('App Middleware refuses to update', this.props, np)
    return false
  }

  render() {
    const { component, defer = false, props} = this.props
    const Component = BuildAsyncComponent( component, defer )
    return <Component {...props} />
  }
}

const BuildAsyncComponent = (component, defer) => 
  createAsyncComponent({
    resolve: component,
    defer
  })

export default connect(
  state => ({
    page: pageSelector(state)
  })
)(AppMiddleware)

With the intent that I would then asynchronously walk down "screens" in the folder structure with a similar structure to the above routes for children routes.

ctrlplusb commented 7 years ago

Hey @bradennapier

This sounds awesome! I don't have time to get into your details now, but I definitely want to go through it all as soon as I can.

FYI I pushed another release, it includes a fix for hot module reloading. It may have been impacting you.

https://github.com/ctrlplusb/react-async-component/releases/tag/0.1.2

ctrlplusb commented 7 years ago

Hey @bradennapier

I had a brief look at your code, key word "brief", but my gut tells me that you should consider creating the AsyncComponent instances within your routes definition.

// shared/app/routes.js

export default [
  {
    id: 'Home',
    exactly: true,
    props: {
      title: 'Home'
    },
    pattern: '/',
    component: createAsyncComponent({
      resolve: () => import('./screens/Home')
    })
  },
  {
    id: 'SecurityCenter',
    props: {
      title: 'Security Center'
    },
    pattern: '/security-center',
    component: createAsyncComponent(
      resolve: () => import('./screens/SecurityCenter')
    })
  }
]

I don't know how well this helper will operate when being executed in the lifecycle methods of your components. This is because the helper tries to attach a determinable identifier to each async component instance so that I can try and track which ones have been resolved without using Babel/Webpack hacks.

Is your reasoning for using the AppMiddleware to create the instances because you wish to dynamically determine whether a route should be deferred or not?

bradennapier commented 7 years ago

EDIT: This got pretty long so sorry to take up so much of your time! If you don't have time to respond I understand, no worries!

I want to automate building of routes by building nested matching and resolving as we move deeper into the directory path.

I think I am fairly close at this point. Basically I want to walk through "screens" and build nested routes dynamically.

image

So in this directory structure I have "shared" which are saved components among all deeper-level components (they can reference anything in that folder as if it were a node_module).

Screens are for routes essentially, they will possibly nest deeper screens or an actual component:

image

So in the above "UserProjects" screen we are routing to it using /project

{
    id: 'UserProjects',
    props: {
      title: 'Project Select'
    },
    pattern: '/project',
    component: () => import('./screens/UserProjects')
  }

Right now I have the /screens/UserProjects/index.js look like this:

import React from 'react'
import Resolver from 'app/resolver'

const routes = [
  {
    id: 'ProjectSelect',
    pattern: '/',
    exactly: true,
    component: () => import('./screens/ProjectSelect')
  },
  {
    id: 'ProjectDashboard',
    pattern: '/dashboard/:projectSlug',
    component: () => import('./screens/ProjectDashboard')
  }
]

export default props => (<Resolver {...props} routes={routes} />)

So this is then dynamically built and says that /project should go to ./screens/ProjectSelect while /project/dashboard/:projectSlug should go to ./screens/ProjectDashboard/.

I could then have the deeper paths return another <Resolver routes={routes} /> to build deeper paths within the directory itself. This allows my directory structure itself to define the routes without dealing with a huge match page that gets out of hand as the app grows.

Right now <Resolver ... /> looks like this:

import React, { Component, PropTypes } from 'react'
import { connect } from 'react-redux'
import { createAsyncComponent } from 'react-async-component'
import { Match, Miss } from 'react-router';

const RoutedResolver = ({ routes, ...props }) => (
  <div>
    {
      routes.map( route => {
        if ( ! route.id ) { throw new Error('Route Does not have an ID: ', route) }
        const matchProps = {
          ...route,
          key: route.id,
          exactly: route.exactly == true,
          pattern: route.pattern === '/'
            ? props.pathname
            : props.pathname + route.pattern
        }
        return <Match {...matchProps} render={ renderProps => {
            const appProps = {
              ...matchProps,
              ...renderProps,
            }
            return <Resolver {...props} {...appProps} />
          }} 
        />
      })
    }
  </div>
)

const Loading = () => (<div>LOADING</div>)

const Resolver = ({ component, defer = false, routes, ...props }) => {
  const Component = routes
    ? RoutedResolver
    : component
      ? BuildAsyncComponent(component, defer)
      : Loading
  if ( routes ) { props.routes = routes }
  return <Component {...props} />
}

export const BuildAsyncComponent = (component, defer) => 
  createAsyncComponent({
    resolve: component,
    defer
  })

export default Resolver

This does appear to work so far, I am sure there are some parts I will need to work on but my worry is how it ends up being handled by code splitting etc - especially when considering the nested routes end up requiring two imports, one for the route then another for the path.

The goal here is to determine within resolver whether we need to build new routes via <Match /> or if we should build the component using BuildAsyncComponent.

This becomes fairly ideal as it also outlines Smart vs Dumb very well. A "Smart" component will always be the final resolved Component and its dumb components will then be rendered from there within the components directory:

image

Sorry to write a book here, just trying to make sure the general concept laid out here isn't going to cause any major issue with how the resolving and handling of the async components work!

ctrlplusb commented 7 years ago

A few more bug fixes have landed @bradennapier - one that could have especially been affecting you regarding component id cycling.

bradennapier commented 7 years ago

FYI my app is sstill loading twice, even using vanilla configuration (Home Mounts is the Home route). It loads the entire application then asyncComponents destroys it and restarts it again

image

bradennapier commented 7 years ago

I added a log here to show when async runs

image

ctrlplusb commented 7 years ago

Any chance you could share a working model so I could try debug it for you?

ctrlplusb commented 7 years ago

Well, when I say "working" I mean an executing example that demonstrates the issue

bradennapier commented 7 years ago

Just invited you to the project, shouldn't be any top secret stuff there yet - just transferred it all over to the next branch etc

ctrlplusb commented 7 years ago

πŸ‘Œ

marella commented 7 years ago

I am facing similar issue when trying to use this library in an existing application (browser-side). componentWillMount() is getting called twice. In below example App is getting called twice - once before withAsyncComponents callback and once after. I fixed it using a wrapper component and an extra variable but it is just a hack.

let readyToRender = false

const App = () => {
  console.log('This will be printed twice')
  if (!readyToRender) {
    return null
  }
  console.log('This will be printed once')

  return <MyApp />
}

withAsyncComponents(<App />)
  .then(result => {
    readyToRender = true
    const { appWithAsyncComponents } = result
    render(appWithAsyncComponents, document.getElementById('app'))
  })
ctrlplusb commented 7 years ago

Ah, okay, I see now. This is by design. I'll update the docs to reflect why this occurs. It will be good to be aware of this moving forward.

What type of things are you doing in componentWillMount? Prepping state?

marella commented 7 years ago

Yes mainly network calls to fetch data. It is making same network calls twice. Is the solution I had the only workaround for this or is there a better way to handle this?

ctrlplusb commented 7 years ago

Is your React application a browser only app?

marella commented 7 years ago

Yes

ctrlplusb commented 7 years ago

A bug! New release is out. You were experiencing behaviour meant for server side execution only. Apologies.

marella commented 7 years ago

Thanks. Working fine now in browser without any hacks.

bradennapier commented 7 years ago

This should fix mine as well? Nice!

bradennapier commented 7 years ago

It actually had its benefits. It allowed me to make sure my app was able to handle situations where for some reason parts of it had to re-render that wasn't expecting it πŸ‘

ctrlplusb commented 7 years ago

Sorry about the confusion @bradennapier

bradennapier commented 7 years ago

Haha yeah it still double renders for me but I am guessing that is cause its SSR and its meant to do it that way?

ctrlplusb commented 7 years ago

Yep, for SSR this is expected/needed. I will update docs with reasoning around this.

bradennapier commented 7 years ago

Yeah I would imagine it has to do with being able to dive deeper into the tree and get the necessary DOM

ctrlplusb commented 7 years ago

Ok, closing this as we continue a more focused discussion on #9