DISCUSSION: Feature ideas for react-server & react-server-cli

[aickin]

(Dunno if this is the right place to put this; please advise if it's not.)

I was thinking about how I could best contribute to making react-server a public success, and I wanted to open a little bit of a discussion about what features might be added to (or subtracted from!) react-server-cli and react-server. I brainstormed for a little while on my own, and the list is pretty scattered and wildly biased towards my own priorities. It's really only meant to be a starting point, and to prevent me from coding on something for a week or two that has no chance whatsoever of being merged because it's not in line with y'all's vision. Caveat: I'm not even sure that these are good ideas if there were infinite time to implement, and they betray my two main priorities: making dev rampup as quick & painless as possible, and having react-server(-cli) be a good example of web performance best practices.

Without any further ado, here's my list, with guesses at T-shirt size:

• Replace superagent with fetch (L/XL): seems like fetch is rapidly becoming the standard, with pretty good polyfills for both client and node. Swapping out superagent for fetch would reduce the amount of documentation required for data loading: rather than sending in a loader argument to the page API and then explaining superagent, we'd just say "use fetch like you already do".
• Port to webpack 2 with tree shaking (M/L): This could in theory reduce the size of the initial bundle. Webpack 2 is not out of beta, yet, though, so it's somewhat moot until then.
• Add support for HTTP/2 with server push (XL/XXL): I think one of the most powerful things about react-server is that the server-side knows, in a structured way, about all of the associated resources for a page (JS, CSS, JSON XHR) and could push them pro-actively. Not only would this theoretically be faster than the current code, it would also obviate the need for the TritonAgent cache, which would simplify the logic a lot in the HTTP/2 case.
• Commons chunk (S): Tweak the webpack build in react-server-cli to pull out a common chunk of JavaScript, which would include the core of react-server. This would allow that chunk to be cached and shared in between pages. (Edited: Forgot I already did this, and it was merged in e30947a. Too much context switching!)
• Long-term caching of static assets (M/L): Implement the webpack config best practices for setting a far future expiration on static assets.
• Allow RootElements to each have their own CSS (XL) - This one is more of a research project in all honesty. It's based on this article by Jake Archibald about CSS link tags in the body, and how they can reduce the amount of time your page spends waiting for CSS before rendering. It's hard for me to see how this would be done with the current API of react-server and webpack, but I haven't spent a ton of time thinking about it.
• Port the react-server tests to use react-server-cli (M) - A bunch of tests for react-server fire up an actual express server, parse and massage the routes file, and run webpack before running a simulated browser against the resulting site. It would be nice if instead they used react-server-cli to do that, both because we could delete a lot of code, and because it would help serve as a test for react-server-cli.
• Allow react-server to be launched with a single Page file instead of a routes file (M) - One of the really interesting ideas at React Conf was a command line that could take in just a component file and would set everything up for you for rapid prototyping. I would love to be able to just point react-server-cli at a page file and have it set up a simple "/" route for that page. I could also potentially imagine pointing it at a directory and having every file named XXXPage.js being mapped to /xxx.
• Allow routes to map to a component instead of a page (M/L) - This is another feature for folks who just want to get a rapid prototype up and running. If you don't care about page title or stylesheets or extra JS, it's a bit of a burden to create a Page class in addition to your component.
• Add support for best quality compression (S) - I've got a fun side project called shrink-ray that is adding zopfli and brotli support to express compression. I don't think this is a particularly good thing to spend time on right now, fwiw.
• Add support for streaming rendering (M/L) - I also had to put this in, as I wrote react-dom-stream; probably also not a particularly good use of time right now until rds is a bit more stable.
• Simplify the use of routr (S) - Routr could be a little more user friendly if it had some smart defaults. For instance, it'd be nice if it didn't require you to always specify the HTTP method of every route, but rather just assumed GET.
• Explore using react-router for routing instead of routr (XL) - I'm super uncertain about this one, but I do think that react-router has become the standard for routing in the React world, and there are a lot more folks familiar with it than with routr. I'm afraid that react-router makes some fundamental assumptions that go against the streaming nature of react-server, but that may have changed, and I may be wrong.
• Add more documentation and example projects about how to do data loading (M) - I may be wrong, but I think there isn't much info on how to use TritonAgent.
• Expose a programmatic API to react-server-cli (S) - Basically just make startServer callable.

OK, that's what I've got for now. As I said, all totally up for discussion and not all great ideas. Please feel to add, subtract, disagree, etc. I'm especially interested in which ideas are total non-starters, and which you'd gladly accept as contributions if they were done well. Thanks!

[kcjonson]

There are a couple things in this list that get me excited. As you may expect they're around HTTP/2, Bundling, and CSS. However, my hunch is that we should carefully consider the order in which we take a look at a couple of these. Heres my two cents:

Port to webpack 2 with tree shaking

• Tree shaking is awesome but good linting goes a long way for blocking unused imports, especially if everything is on ES6 syntax 😉
• Seems like we should do this before we look at RootElements having their own CSS. Highly theoretical but we could use the new System.import syntax to write CSS dependencies in as RootElements are encountered, or even resolved.
• If we're planning on upgrading to webpack 2, seems like we should do it first since there were "major changes" to the resolver which might effect other work. However, its in beta so are we ok with the following items being held up by it?

Add support for HTTP/2 with server push

• When we took a look at this one at a Hackathon, Redfin was limited by our CDN, Akamai, on actual HTTP/2 deployment. I'm SUPER SUPER excited about HTTP/2 and the benefit for end users, so I'd love to help out with this in any way possible.
• For this generic body of work, do we want to consider killing CSS and JS bundles for HTTP/2 requests? We'll still need to look at the dependency tree for a given route so that we know what to push, and we'll probably still need bundles deployed for non HTTP/2 browsers.
• What does this mean for our loaders on the client? How does it relate to the other work item of allowing RootElements to have their own CSS?

Allow RootElements to each have their own CSS

• This seems super dependent on webpack, so we might want to do the Webpack 2 upgrade first. Highly theoretical but some approaches to this may have us using dynamic expressions in System.import
• Do we still use webpack to create the common chunk? How does the work across routes and root elements? If each root element is an entry point then the only things that would end up in the common bundle are things that appear in every root element, which is most likely not the behavior we want.
• Do we still use webpack to compute the list of dependencies for a given root element? A first baby step could be to get webpack to create a bundle for each root element and go from there.
• Are the <link> tags for each file in the entire css dependency tree for the root element written into the body before the element? Would be messy to look at but in HTTP/2land it may be the right answer.
• Related note: Recently @roblg took a look at moving the LESS processing and preamble prepending out of webpack into a gulp step which could in theory reduce the complexity of Redfins webpack config and afford us more flexibility when it comes to implementing HTTP/2

[aickin]

Thanks for the feedback and enthusiasm, @kcjonson!

A few unprioritized reactions:

Tree shaking is awesome but good linting goes a long way for blocking unused imports, especially if everything is on ES6 syntax

I think you probably already know this, but tree shaking goes one step further than linting. For example, right now if you code

import { Link } from "react-server"

you import everything from react-server (~85KB gzipped) into your codebase, even if you are linting. With tree-shaking, you import just Link and its dependencies. I have no earthly idea how much of an effect this would actually have; would probably be best as a research project at first.

However, its in beta so are we ok with the following items being held up by it?

I personally don't think it makes a lot of sense, no. While webpack 2 is still in beta, I think it's an interesting research project, but I probably wouldn't worry too much about its effect on other stuff. Depending on unreleased open source versions is a dangerous game.

For this generic body of work, do we want to consider killing CSS and JS bundles for HTTP/2 requests?

I don't know, and I think it's an empirical question. I did some experimentation with HTTP/2 in Apache in January to see how unbundling would affect a test page, and the results were pretty terrible when large numbers of JS files were unbundled. I don't know what caused my results, though; it could have been HTTP/2 implementation immaturity, bad default settings, increase in size due to gzip inefficiencies when unbundled, the absence of server push in Apache, or something else entirely. Khan Academy wrote a blog post about HTTP/2 unbundling slowing them down, but it's a deeply unsatisfying post from a technical explanation standpoint. Basically, I think we'd have to experiment with differing levels of bundling and see how browsers respond.

We'll still need to look at the dependency tree for a given route so that we know what to push, and we'll probably still need bundles deployed for non HTTP/2 browsers.

Agreed 100% on both counts.

Do we still use webpack to create the common chunk? How does the work across routes and root elements? If each root element is an entry point then the only things that would end up in the common bundle are things that appear in every root element, which is most likely not the behavior we want.

Short answer is that I have no idea. To be clear, I'm not even sure this task is possible with webpack's current feature set. All I'm sure of is that it would require some webpack wizardry combined with some changes to the Page API.

To be frank, I think that RootElements having their own CSS is an interesting feature, but it doesn't seem nearly as important to me as some other more fundamental perf optimizations (e.g. far-future expiration) or smoothing the dev rampup experience (e.g. routes file complexity, requirement to have routes file for simple sites, doc around data loading).

[aickin]

Another Q, @kcjonson: do you have any feelings about things like CSS Modules or PostCSS support?

[roblg]

Replace superagent with fetch
Do you mean replace TritonAgent with fetch? Or you mean still have TritonAgent with the same/similar API, and use fetch under the hood? I'm not really opposed either way. superagent has some quirks that are annoying, and builder patterns are obnoxious to mock in tests.

I also don't quite understand this:

rather than sending in a loader argument to the page API and then explaining superagent, we'd just say "use fetch like you already do".

We currently just have pages require("triton").TritonAgent, and fetch willy-nilly, unless something changed while I wasn't paying attention 😁

Port to webpack 2 with tree shaking
This mostly affects react-server-cli, right?

Add support for HTTP/2 with server push
I have a prototype of this running w/ the http2 module, but it's based on our old internal version of react-server. I think with a little work, I should be able to port it to work w/ this react-server if that's interesting. The main problem I ran into when I was poking at this is that the http2 module expects a certain API for request and response objects, and express 4.x does not have that API -- I had to switch us to use connect instead. I think express 5.x was supposed to have a compatible API, but there's a whole lot of drama in that project, last I checked.

Long-term caching of static assets
This isn't too big of a project - we're doing it internally, implemented as a middleware+a one-line webpack plugin to drop the assetsByChunkName.json (or whatever) file in the build output directory. We ran into some fun issues w/ this though -- webpack's DedupePlugin can cause the output bundles to not change hashes, even if the contents change :crazy:

Port the react-server tests to use react-server-cli
I'd be interested to see this. I noticed while I was going through the react-server-cli PR that a lot of the setup stuff is the same. :) This would probably be easier if react-server-cli exposed a start function, as suggested above.

Simplify the use of routr
Yep.

Explore using react-router for routing instead of routr
I've thought about this briefly a couple times after you and I discussed it months ago. The streaming thing is where it really starts to break down.

Add more documentation and example projects about how to do data loading
Most definitely. We've been tossing around the possibility of open-sourcing our reflux Stores as well, but there are a couple of nasty known bugs that I'd like to fix first (and I don't know how long it's going to be before I get to that...)

[aickin]

Do you mean replace TritonAgent with fetch?

Sorry, yes, this is what I mean.

We currently just have pages require("triton").TritonAgent, and fetch willy-nilly, unless something changed while I wasn't paying attention.

You're right, I was going off some incorrect memories. I think the point about not having to document TritonAgent vs. fetch stands, though.

This mostly affects react-server-cli, right?

Mostly. I think (?) to get the most benefit it might require switching to ES6 imports inside the react-server codebase, though, and mucking with the build so that they aren't transpiled to CommonJS before the webpack build. But otherwise, yes, it's a react-server-cli thing.

I had to switch us to use connect instead.

I remember you saying that. I don't think we depend on anything in express that isn't in connect, but I wouldn't be surprised if I was wrong about that.

I think express 5.x was supposed to have a compatible API, but there's a whole lot of drama in that project, last I checked.

Truth. I went looking for info on express 5 yesterday, and after 3 hours of reading github issues I think it seems like it's not being released.

to drop the assetsByChunkName.json (or whatever) file in the build output directory.

I think this gets a little more complicated if you separate the client build and server build (i.e. you want to pre-compile client code for a static server), but it's still not super hard.

I noticed while I was going through the react-server-cli PR that a lot of the setup stuff is the same. :)

It's almost like the same person wrote them both... ;)

The streaming thing is where it really starts to break down.

Yep, although I think they may have changed a few things to make it better? But I'm not sure?

[kcjonson]

Re: CSS Modules or PostCSS

We should be clear exactly what problem were trying to solve here. The most dangerous thing about css is that all selectors on a page are essentially global to all components. Selectors from any stylesheet can target any dom node on the page. This is bad for scale software projects that have more than one or two people working in the css.

We currently solve this purely by convention. Our convention to make things simple for devs is that we match three things. Forgive the pseudo code below.

Filename: /common/Button.js

import './Button.less';
export default class Button extends Component {
   static displayName: 'Button';
   render() {
      return(<button className='Button'>...</button>);
   }
}

Filename: /common/Button.less

.Button {
  color: blue;
}

The first thing I see with this is that the string "Button" appears seven times. Lets count them:

1. The name of the JS file
2. The name of the LESS file
3. The import statement of the less file
4. The javascript class declaration
5. The component display name
6. The className on the root element of the component
7. In the stylesheet as a parent of all selectors

Our convention is to keep them all in sync. We teach all new hires that they should be in sync by verbally telling them or catching the pattern in their first pull requests. We really haven't even documented it anywhere (which is totally my fault btw) This is certainly a broken system in my opinion. It's learning process is broken, there are no tests to enforce it, and major regressions are regularly caused when its not adhered to.

Now that we're all pissed off at how silly we're being with CSS and how much work we're causing the devs just trying to do daily feature work , what can we do about it?

• We could create our own base module or build system that does things such as automatically adding its classname to the root element of the template, setting its display name to its class name, and possibly even importing a less file that has the same name and is a sibling.
• There are also a systems that automatically create and apply a unique class name to the root element of the component and wrap the components css file with the same selector. Webpack and CSS modules can sorta do that, but its not 100% automatic, most that I've seen still rely on some sort of :local { syntax in the css.
• I would support a linter setting that insists on keeping all seven of those names matched up as a matter of convention if we don't build an automated solution to match class names and selectors up.

So, you mentioned PostCSS of which the most relevant part to trying to solve the aforementioned problem of styles seeking into the global scope is the react-css-modules plugin. Its base webpack only implementation is such:

import styles from './table.css';
export default class Table extends Component {
    render () {
        return <div className={styles.table}>
            <div className={styles.row}>
              // More stuff here
            </div>
        </div>;
    }
}

Which results in:

<div class="table__table___32osj">
    <div class="table__row___2w27N">
        <!-- More stuff here -->
    </div>
</div>

A couple things that I see about this approach. It certainly does solve the problem of styles leaking into the global scope, which really should be our number one issue. Still, there is a lot of convention, and a it doesn't really solve anything about the dev annoyances either. I don't personally find it easier to understand or reason about, but I could see myself getting used to it. There are a couple things you can do outside of webpack in the JS code to clean it up a bit, but I still find it a bit cumbersome and messy.

To be perfectly clear, I do believe that adopting react-css-modules will reduce the number of global style regressions at Redfin.

So, here's where I'll add some controversy and some open questions. To me, most of solutions out there don't do enough to solve the dev annoyances part. Here is an extreme proposal, that addresses all of the issues that I have mentioned above.

Source Files

Filename: /common/Button.js

export default class extends Component {
   render() {
      return(<button>...</button>);
   }
}

Filename: /common/Button.less

color: blue;

or alternate syntax

:local {
    color: blue;
}

Get transformed at build time into:

import './Button.less';
export default class Button extends Component {
   render() {
      return(<button className="Button x0ns8">...</button>);
   }
}

.Button.x0ns8 {
   color: blue;
}

Which ultimately outputs:

<button class='Button x0ns8'>...</button>

.Button.x0ns8 {
   color: blue;
}

• I don't think that everyone will like this, because its too damn magical.
• The source JS is still a 100% valid ES6 module, there aren't even any special imports or syntax. It won't have any styles on it if its used directly. If we want to swap out for other build processes in the future, our entire codebase isn't caught up in a syntax that is very specific to a particular loader or transpiler.
• The javascript class name should be automatically added to the root dom node, all other classes need to be left there too.
• A unique id should also be inserted as the base class, but not with any weird underscore syntax or duplication. I find table__row___2w27N incredibly janky. I feel strongly that we should allow people to write normal global css and those crazy custom class names make it impossible, we really don't want to lose the ability to provide good global defaults on things.
• If a dev really wants to write the declaration as export default class Button extends Component we should totally allow that, its more correct.
• If the default export is anything other than anonymous or "Button" there should be a linter and build failure.
• The import statement for the CSS will be inserted automatically if there is a css file of the same name as a child. Other css import statements will be allowed and treated as normal. Manually importing a file of the same name should also work.
• The css file should be wrapped in a selector that targets the javascript class name and the unique identifier.

Anyway, those are my initial thoughts, but I'll let people chew on that for meow.

[aickin]

Port the react-server tests to use react-server-cli

@roblg: one other question about this. I can't exactly figure out what the package dependencies would look like. I think that since react-server-cli depends on react-server, we'd have to move all the tests from react-server out into their own project that could take both react-server-cli and react-server as dependencies. A bit ugly to not have the tests right next to the code, but I can't see another way. Does that seem correct to you?

[DanielFGray]

Personally, and I'm just gonna throw this out there, I would love to see a future version of this that uses Koa instead of Express.

[doug-wade]

@DanielFGray This issue is getting pretty large; would you mind filing the Koa issue on its own?

[doug-wade]

This issue is too big for my puny brain; I'm going to split it into individual issues.