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

Convert routes into chunkable routes #129

Closed
wants to merge 0 commits into from

Conversation

declanelcocks
Copy link

@declanelcocks declanelcocks commented Feb 23, 2017

In response to #128. Just an initial PR as I imagine there could be some things to improve with my logic.

Note: This will still include one main chunk containing all components since we are exporting all components in the index.js file. Still need a way to solve this before the app can be properly chunked.

@codecov-io
Copy link

codecov-io commented Feb 23, 2017

Codecov Report

Merging #129 into fullstack will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           fullstack   #129   +/-   ##
========================================
  Coverage        100%   100%           
========================================
  Files             82     82           
  Lines            802    802           
  Branches         175    175           
========================================
  Hits             802    802

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 710397e...2d396ee. Read the comment docs.

@declanelcocks
Copy link
Author

declanelcocks commented Feb 24, 2017

Note that the hot loader is not working due to the way I'm now rendering the app client side, so will have to find a better way to structure the client file.

@diegohaz
Copy link
Owner

Thank you, @declanelcocks

Will review that this week

src/client.js Outdated
<AppContainer>
<Provider store={store}>
<Router history={history} routes={routes} />
<Router {...renderProps} />
</Provider>
</AppContainer>
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const renderApp = () => {
  match({ history, routes: routes, location }, (error, redirectLocation, renderProps) => {
    render(
      <AppContainer>
        <Provider store={store}>
          <Router key={Math.random()} {...renderProps} />
        </Provider>
      </AppContainer>,
      root,
    )
  })
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hot loaded works this way

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kybarg Didn't know that. Does it fixes HMR on the *Page components?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegohaz yes, it works

Copy link
Owner

@diegohaz diegohaz Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the match call necessary here? I wonder if placing just key={Math.random()} would not solve HMR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegohaz haven't tried it. Without key={Math.random()} there is warning about adjusting routes after HMR is called.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kybarg Thanks 😄 I didn't know you could add a key property to the <Router /> component. I was scratching my head trying to think how to solve this warning about changing routes when HMR is called.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@diegohaz When I don't use the match call, the app will successfully render on the client, but it will throw the above warning in console. The reason I added the match call was to try and see how I can remove this warning to make sure the client/server rendered pages are the same. My understanding of exactly what is happening with the render or match calls is not perfect, so would be great to get some input on that.

src/client.js Outdated

match({ history, routes, location }, (error, redirectLocation, renderProps) => {
render(renderApp(renderProps), root)
})

if (module.hot) {
module.hot.accept('routes', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (module.hot) {
  module.hot.accept('routes', () => {
    renderApp()
  })
}

renderApp()

src/server.js Outdated
let content

if (isDev) {
// Place content in `AppContainer` in development
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this isDev logic now. Since the HMR is fixed it's no longer needed.

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

Successfully merging this pull request may close these issues.

4 participants