ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.7k stars 244 forks source link

Merge server and universalMiddleware #194

Closed ctrlplusb closed 7 years ago

ctrlplusb commented 7 years ago

Pretty big change here, and may annoy a few of you whom have been doing a lot of toolchain experiments based on the current configuration... sorry.

I have been playing around with a merge of the server and the universalMiddleware as the tools have evolved a lot since I did the original break out. It appears the matured toolchain is much more able to support an integrated middleware approach. I am seriously considering doing this change as it simplifies the project a lot, and simple is always better.

There are some really nice things that come along with this:

Thoughts?

Don't hate me please. 😇

ctrlplusb commented 7 years ago

Note: not 100% committed to this yet. Will try it out against a complex project of my own, one which has complex data fetching logic applied to the components. This case may have its hot reloading process interrupted by a server restart.

enten commented 7 years ago

Why you splitted server and universalMiddleware originally?

And why you reverse that choice? Can you detail your motivations to merge server and universalMiddleware again?

I'm not an expert with webpack stuffs (hot reloading, code spltting, and so). And I don't want to lose time on issues that you already solved.

strues commented 7 years ago

I think this sounds like a good idea in theory. Simplicity is always preferred. Curious to see what your approach looks like.

ctrlplusb commented 7 years ago

Good news! It works with a complex application configuration! I am going to move forward with this.

@enten:

I originally did the split as the server used to restart in the middle of a webpack hot reload request being executed by the client, which would cause the hot reload request to fail. I had all sorts of strange edge cases around this. I think it was because my original configuration was overly complex and buggy.

Merging is purely out of the desire to keep things as simple as possible. This is the only starter/boilerplate that I know of which had this server/middleware split. I think it will help people understand and adopt this starter kit if I simply have a client/server set of bundles along with a single shared folder.

ctrlplusb commented 7 years ago

On the next branch.

enten commented 7 years ago

I originally did the split as the server used to restart in the middle of a webpack hot reload request being executed by the client, which would cause the hot reload request to fail.

I don't figure how a hotServer restart can crash hot reloading because the socket used for hot reloading is opened with the hotClient server.

Merging is purely out of the desire to keep things as simple as possible.

Well! So it was necessary to merge again.

ctrlplusb commented 7 years ago

I don't figure how a hotServer restart can crash hot reloading because the socket used for hot reloading is opened with the hotClient server.

Yeah, that is a good point, I can't remember exactly how and why my configuration created this strange dependency. It may have also only been introduced with advanced component configurations making calls to a server api. I might go back and investigate at some point the exact details out of interest sake. I remember it being a painful time for me, and the introduction of the universalMiddleware made my life a happy place again. :) Most likely it may have just been a bad toolchain configuration or a bug in my code somewhere that was the root cause.

codepunkt commented 7 years ago

@ctrlplusb I've been working my way through the amazing toolchain you've set up in the last few days. Amazing work - especially on the rather verbose comments. Those help a lot!

Simple is good! I've always wondered why there are node/universal subfolders in shared. It is commented, like most things are - but i couldn't make sense of it. Same for the universalDevMiddleware! 👍

enten commented 7 years ago

electrode was released since one month. That may be a good reference for the merging work and more.

I liked electrify as stats analyzer, electrode-server to manage hapi server easily, confippet to manage configurations and webpack-isomorphic-loader has awakened my curiosity.

It's a good thing to know that wallmart's framework in case where we hurting on issues already solved by them.

What do you think about the webpack-isomorphic-loader? Can it improve react-universally boilerplate?

ctrlplusb commented 7 years ago

I had a look at the webpack-isomorphic-loader, it is pretty cool for getting off the ground quickly, but I felt like it abstracted away the webpack configuration to the point where I didn't properly understand what was going on. After I tried to create a universal/isomorphic application using a standard webpack config I realised it isn't actually that difficult. I like having the full webpack configs exposed as it makes editing them far easier, as I can understand fully what is going on. Additionally when I used the webpack-isomorphic-loader it required that webpack be executed during runtime to generate the bundle that handles server side rendering. I prefer to do pre-compilation/bundling.

I haven't heard of electrode. It looks pretty interesting, thanks for the link! :)

enten commented 7 years ago

when I used the webpack-isomorphic-loader it required that webpack be executed during runtime to generate the bundle that handles server side rendering. I prefer to do pre-compilation/bundling.

+1