fix($misc): fix HMR, remove onBefore from server, dont call both flushing techniques simultaneously

Author: faceyspacey

README.md

@@ -245,7 +245,7 @@ MyUniversalComponent.doSomething()
 
 - `isLoading: boolean`
 - `error: new Error`
-- `onBefore`: `({ isMount, isSync, isServer }) => doSomething(isMount, isSync, isServer)`
+- `onBefore`: `({ isMount, isSync }) => doSomething(isMount, isSync)`
 - `onAfter`: `({ isMount, isSync, isServer }, Component) => doSomething(Component, isMount, etc)`
 
 ### `isLoading` + `error`:
@@ -280,9 +280,9 @@ export default graphql(gql`
 
 ### `onBefore` + `onAfter`:
 
-`onBefore/After` are callbacks called before and after the wrapped component changes. It's also called on `componentWillMount` on both the client and server. If you chose to use it on the server, make sure the client renders the same thing on first load or you will have checksum mismatches.
+`onBefore/After` are callbacks called before and after the wrapped component changes. They are also called on `componentWillMount`. However `onBefore` is never called on the server since both callbacks would always render back to back synchronously. If you chose to use `onAfter` on the server, make sure the client renders the same thing on first load or you will have checksum mismatches.
 
-It's primary use case is for triggering *loading* state **outside** of the component *on the client during child component transitions*. You can use its `info` argument and keys like `info.isSync` to determine what you want to do. Here's an example:
+The primary use case for these callbacks is for triggering *loading* state **outside** of the component *on the client during child component transitions*. You can use its `info` argument and keys like `info.isSync` to determine what you want to do. Here's an example:
 
 ```js
 const UniversalComponent = univesal(props => import(`./props.page`))
@@ -294,7 +294,7 @@ const MyComponent = ({ dispatch, isLoading }) =>
     <UniversalComponent
       page={props.page}
       onBefore={({ isSync }) => !isSync && dispatch({ type: 'LOADING', true })}
-      onAfter={({ isSync }) => !isSync && dispatch({ type: 'LOADING', false })}
+      onAfter={({ isSync }, Component) => !isSync && dispatch({ type: 'LOADING', false })}
     />
   </div>
 ```
@@ -303,6 +303,8 @@ Each callback is passed an `info` argument containing these keys:
 
 - `isMount` *(whether the component just mounted)*
 - `isSync` *(whether the imported component is already available from previous usage and  required synchronsouly)*
+
+**`onAfter` only:**
 - `isServer` *(very rarely will you want to do stuff on the server; note: server will always be sync)*
 
 `onAfter` is also passed a second argument containing the imported `Component`, which you can use to do things like call its static methods.

__tests__/index.js

@@ -516,7 +516,7 @@ describe('SSR flushing: flushModuleIds() + flushChunkNames()', () => {
     expect(chunkNames).toEqual(['component', 'component3'])
   })
 
-  it('webpack: dynamic require (babel-plugin', async () => {
+  it('webpack: dynamic require (babel-plugin)', async () => {
     global.__webpack_require__ = path => __webpack_modules__[path]
 
     // modules stored by paths instead of IDs (replicates babel implementation)

src/flowTypes.js

@@ -90,9 +90,10 @@ export type Ids = Array<string>
 // RUC
 export type State = { error?: any, Component?: ?any }
 
-type Info = { isMount: boolean, isSync: boolean, isServer: boolean }
-type OnBefore = Info => void
-type OnAfter = (Info, any) => void
+type OnBeforeInfo = { isMount: boolean, isSync: boolean }
+type OnAfterInfo = { isMount: boolean, isSync: boolean, isServer: boolean }
+type OnBefore = OnBeforeInfo => void
+type OnAfter = (OnAfterInfo, any) => void
 
 export type Props = {
   error?: ?any,

src/index.js

@@ -74,7 +74,7 @@ export default function universal<Props: Props>(
       addModule(this.props) // record the module for SSR flushing :)
 
       if (Component || isServer) {
-        this.handleBefore(true, true, isServer)
+        if (!isServer) this.handleBefore(true, true)
         this.update({ Component }, true, true, isServer)
         return
       }
@@ -96,7 +96,7 @@ export default function universal<Props: Props>(
           this.props
         )
 
-        if (shouldUpdate(nextProps, this.props) || isHMR()) {
+        if (shouldUpdate(nextProps, this.props)) {
           const Component = requireSync(nextProps)
           this.handleBefore(false, !!Component)
 
@@ -114,6 +114,9 @@ export default function universal<Props: Props>(
 
           this.update(state, false, true)
         }
+        else if (isHMR()) {
+          requireSync(nextProps) // just needs to be required again to complete HMR
+        }
       }
     }
 
@@ -150,14 +153,10 @@ export default function universal<Props: Props>(
       this.handleAfter(state, isMount, isSync, isServer)
     }
 
-    handleBefore(
-      isMount: boolean,
-      isSync: boolean,
-      isServer?: boolean = false
-    ) {
+    handleBefore(isMount: boolean, isSync: boolean) {
       if (typeof this.props.onBefore === 'function') {
         const onBefore = this.props.onBefore
-        const info = { isMount, isSync, isServer }
+        const info = { isMount, isSync }
         onBefore(info)
       }
     }

src/requireUniversalModule.js

@@ -119,6 +119,12 @@ export default function requireUniversalModule<Props: Props>(
 
   const addModule = (props: Object): void => {
     if (isServer) {
+      if (chunkName) {
+        const name = callForString(chunkName, props)
+        if (name) CHUNK_NAMES.add(name)
+        if (!IS_TEST) return // makes tests way smaller to run both kinds
+      }
+
       if (isWebpack()) {
         const weakId = callForString(resolve, props)
         if (weakId) MODULE_IDS.add(weakId)
@@ -127,15 +133,6 @@ export default function requireUniversalModule<Props: Props>(
         const modulePath = callForString(path, props)
         if (modulePath) MODULE_IDS.add(modulePath)
       }
-
-      // just fill both sets so `flushModuleIds` continues to work,
-      // even if you decided to start providing chunk names. It's
-      // a small array of 3-20 chunk names on average anyway. Users
-      // can flush/clear both sets if they feel they need to.
-      if (chunkName) {
-        const name = callForString(chunkName, props)
-        if (name) CHUNK_NAMES.add(name)
-      }
     }
   }