Skip to content
This repository has been archived by the owner on Dec 23, 2021. It is now read-only.

[WIP] Migrate header #182

Merged
merged 11 commits into from
Aug 4, 2016
Merged

[WIP] Migrate header #182

merged 11 commits into from
Aug 4, 2016

Conversation

clkao
Copy link
Contributor

@clkao clkao commented Aug 3, 2016

For the actions to work in universal environment, i think the rehydration needs to rerender everything so handlers with send() will work.

Closes #168.

@clkao
Copy link
Contributor Author

clkao commented Aug 3, 2016

It seems app.start always gets a fresh state object and not using whatever the server serialized with toString(): choojs/choo#141

@laurengarcia
Copy link
Contributor

laurengarcia commented Aug 3, 2016

Regarding choo's issue #141 that you mentioned about, it's actually not really choo's responsibility to rehydrate itself from the state that was passed into app.toString() from the server. There's no magics there (and I agree -- that's not really choo's responsibility). I just chatted with Yoshua to confirm as well; choo issue 141 is just about the mechanics of the rehydration, but does not actually represent the issue you described. We should rehydrate from a special object that we write into the document "manually". Yoshua also confirmed that this is what other ppl are doing too. That's why I opened this issue, because I haven't actually bridged the gap yet, needs to be done --> #163

I want to pull this down and play with it before merging this PR to make sure its exactly what I want, am with the team and will do that in a few hours or maybe later tonight.

@@ -1,14 +1,10 @@
const html = require('choo/html')
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with moving /server/pages/archive/index to client/js/pages/... this app will not be truly isomorphic in the sense that it will be identical on server and client. We will have pieces that are server-rendered and pieces that are client-rendered from rehydration, and the only possible overlap might be certain states of the header, but for the most part the header should be rendered on the server, and so should the basic markup for the page-level template. So moving this to the client doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but i think we should diverge the render-ness logic in the component level rather than the entry-point to make things simpler. for example: the hyperdrive-ui bit can be rendering the html tree plus a tree object in server, while in client side it's rehydrated with yofs on the tree object, and connects to peer for updates. Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see your point! Put me down for +1 --> agree.

@okdistribute
Copy link
Collaborator

wow good looks like things are choo-ing along!

@okdistribute
Copy link
Collaborator

https://blog.kaspersky.com/files/2015/12/train-hack-featured-1.jpg

@clkao
Copy link
Contributor Author

clkao commented Aug 3, 2016

right, it's a different issue to serialize whatever the store has after server-side rendering, and have that state populated in the client. this is often with a magic object injected during server rendering in the redux world: https://github.com/erikras/react-redux-universal-hot-example/blob/master/src/helpers/Html.js#L52

@laurengarcia
Copy link
Contributor

clkao: yes, that react example is exactly what (IMO) we should do (for now). it's kind of a hacky approach tho, and in the long term, we should rehydrate from an xhr call and aggressively cache the server-rendered templates (without the state object in them). I'll add that nice example you found to the parent issue about the rehydration.

@juliangruber
Copy link
Contributor

in the long term, we should rehydrate from an xhr call

i don't think it's really hacky to do the <script>state = ${JSON.stringify(state)}</script> thing, it saves you one roundtrip which especially on bad internet will be way faster.

@laurengarcia
Copy link
Contributor

Yeah, Yoshua and I were discussing this as well and I agreed with you, @juliangruber, but then Yoshua had a great point, which is that if we do xhr to rehydrate, we can then aggressively cache the page-level templates with a CDN. If you want to chime in, I started a discussion laying out the pros and cons in #163

@laurengarcia
Copy link
Contributor

weird -- Travis is reporting e2e tests failing, but I ran them locally twice and there is no problem. All passing.

@laurengarcia laurengarcia merged commit ba50d1d into master Aug 4, 2016
@max-mapper max-mapper deleted the migrate-header branch December 13, 2016 00:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants