Implement custom async primitive #17

divmain · 2017-02-08T02:52:59Z

This PR removes reliance on Most.js streams in favor of a custom Sequence implementation.

I also experimented with a generator-based approach (see here). This drastically improved readability, but damaged performance by 25-40% compared to the original. In comparison, this PR makes things somewhat less readable :( but improves performance by another ~50% over the original implementation on concurrent renders and also closes #7.

This also brings a breaking change to the public API - but, seeing as basically nobody is using this yet, that seems a non-issue. And I'd say that the new API is just as nice as the original.

API changes are reflected in the README.

/cc @kenwheeler @aweary

@mhaagens, would also like your eyes on this since you're the first user!

…ctions.

…marks.

ryan-roemer

Code quality and idioms look great!

I'll leave it to @kenwheeler and @aweary for the substantive correctness details.

It would be awesome if we could get jsdoc-style comments on the major functions and classes (maybe a future ticket?) as I'm not deeply versed in this stuff and having a bit of difficulty just following the literal code. (Maybe it's just me).

Otherwise, just a couple of minor nits!

ryan-roemer · 2017-02-08T04:52:18Z

README.md


-This function translates [Most.js](https://github.com/cujojs/most) streams to Node streams.  You'll be able to pipe the output of these Node streams to an HTTP Response object or to disk.
+This function evalues a React VirtualDOM Element, and returns a Node stream.  This stream will emit string segments of HTML as the DOM tree is asynchronously traversed and evaluated.


NIT: s/evalues/evaluates/

ryan-roemer · 2017-02-08T04:54:58Z

README.md


-However, you may want to change this number if your server is under heavy load.  Possible values are the set of all positive integers.  Lower numbers will be "more asynchronous" (shorter periods between I/O processing) and higher numbers will be "more synchronous" (higher performance).
+Note that the template composes both a stream of HTML text (`componentHtmlStream`) and a function that evaluates to the store's state - something you'll often want to do with SSR.


NIT: I think it reads better as:

"the template is composed of" OR

"the template comprises"

ryan-roemer · 2017-02-08T05:03:12Z

package.json

-    "test": "npm run lint && npm run test-only",
-    "test-only": "mocha spec/exec.js",
+    "check": "npm run lint && npm run test",
+    "test": "mocha spec/exec.js",
    "lint": "eslint ./src ./spec",


GLOBAL NIT: If you remove ./ prefixes, the npm tasks will be windows compatible.

Really... well that's useful info. Will do!

aweary

Added a few comments! As far as the renderer goes it looks pretty good to me. The streaming/caching modules were a little hard to follow so I'm not entirely sure I can provide any substantial feedback on those yet.

aweary · 2017-02-08T20:36:47Z

benchmark/_util.js

@@ -12,16 +15,38 @@ const alternateColor = text => {
  return _color(text);
 };

-const time = (description, fn) => {
+
+const time = (description, fn, baseTimeOrFn) => {


It might be worth using an existing benchmark library like benchmark.js instead of rolling our own.

aweary · 2017-02-08T20:38:37Z

benchmark/benchmark.js

 import { time } from "./_util";


+// Make sure React is in production mode.
+process.env.NODE_ENV = "production";


I'm not 100% sure if it matters, but we probably want to set this before we require in React/ReactDOMServer. Maybe just add it to the npm script?

"benchmark": "NODE_ENV=production babel-node benchmark/benchmark.js"

renderToString performance was considerably different after I made this changed, compared to before. I'm not entirely sure what do do with the benchmark/ dir - its not all benchmarks, its really just a collection of manual tests that have helped me along... But since there is a script entry for it in package.json, that seems like a reasonable change to make!

Yeah it seemed like it made the appropriate change, but I figure setting the ENV variable before anything executes would future-proof it in case there were any future changes to React (or any consumed module in benchmark) that implemented conditional exports based on process.env.NODE_ENV, which isn't unheard of.

aweary · 2017-02-08T20:43:03Z

package.json

-    "lint": "eslint ./src ./spec",
+    "check": "npm run lint && npm run test",
+    "test": "mocha spec/exec.js",
+    "lint": "eslint src spec",
    "benchmark": "babel-node benchmark/benchmark.js",


babel-node might be skewing the results of the benchmark. It's not meant for production as it does a lot of in-memory caching, and that might affect the results.

If we remove the ESM imports/exports and just use React.createElement instead of JSX I think we can run it with node directly.

I can double-check to make sure, but I think this is okay as-is. babel-node will transpile the benchmark.js source and all of its dependencies when node originally starts. That shouldn't effect benchmark times (since Node is compiling the transpiled bytecode by that point), and the extra memory usage shouldn't be an issue either.

I'll switch the benchmark over to use React.createElement just to be sure - if the numbers don't change, I'll leave it as-is.

aweary · 2017-02-08T20:45:07Z

src/consumers/promise.js

+
+function toPromise (gen, batchSize) {
+  const arrayBuffer = [];
+  return new Promise(resolve =>


How performant are native promises in Node these days? Could using bluebird instead potentially increase performance?

Hmm... good question. Since this is a simple promise, the performance difference is going to be negligible. bluebird really shines when composing lots of promises, relying on them for everything async (like in Interlock). So... probably fine? I could make the change to the benchmark, see if it makes a difference.

aweary · 2017-02-08T20:52:33Z

src/render/traverse.js

+    }
+  }
+
+  return attrString.join("");


Have you considered using string concatenation instead of arrays? I benchmarked it and string concatenation seems to be much faster: https://esbench.com/bench/589b83a999634800a03477e7

I have a renderer I was working on and found string concat. was much faster in Node as well.

Really... I thought the opposite was true! In that case, I'd be very interested in switching it out. Thanks for the tip!

Wow! That's a crazy difference. I'll make this change for sure!

It's implementation specific and string concat IIRC is going to be better than arrays in all modern JS / browser engines.

Potentially worth testing on all node's that we intend to support...

aweary · 2017-02-08T20:57:50Z

src/render/traverse.js

+function evalComponent (seq, node, context) {
+  const componentContext = getContext(node.type, context);
+  // eslint-disable-next-line new-cap
+  const instance = new node.type(node.props, componentContext);


Won't this throw if the function is not a constructor, e.g., an arrow function? You can easily determine if a React component should be constructed by checking type.prototype.isReactComponent.

I think doing that check would remove the need for isNullComponent too.

That's interesting... I haven't run into a circumstance where it throws, but I was completely unaware of isReactComponent. That might even perform better...

@divmain have you tested with SFCs written using arrow functions? Since arrow functions dont have the internal [[Construct]] method it should throw if you try to new it.

If the SFC returns a React element, the behavior is identical to cases where the function was invoked directly (Type() vs new Type()). The only exception to this is isNullComponent, which is why that check is there. But if there's a simple boolean check that avoids some of that rigmarole, all the better!

But if the SFC is defined using an arrow function then doing new Type is an illegal operation as arrow functions cannot be constructed. I think the tests/benchmarks might be missing this because babel is compiling arrow functions to regular ones.

Check out this example: https://jsfiddle.net/ydpcmo57/

var SFC = () => React.createElement('div') var element = React.createElement(SFC) // this throws new element.type()

Oh wow! You're absolutely right.

aweary · 2017-02-08T21:03:14Z

src/render/traverse.js

+    return;
+  }
+
+  if (isString(node)) {


It should treat numbers as text as well.

aweary · 2017-02-08T21:06:12Z

src/renderer.js

+
+
+class Renderer {
+  constructor (jsx, sequence) {


nit: it's not really JSX being passed here as its just a syntax extension and we're passing the result of the JSX call. It might be better to call it element or something.

Yeah, agreed.

divmain · 2017-02-08T21:26:25Z

Thanks @aweary! I may make some of the changes you recommended in a subsequent PR, since none of these are blocking and I have a couple of other PRs stacked up on top of this one; #19 and #20.

The Sequence code might be easier to follow once the JSDoc PR is merged.

divmain added 9 commits February 7, 2017 17:27

Remove Most.js attribution :(

beae838

Move .babelrc to available everywhere.

4c14677

Add new Sequence async primitive.

8eee036

npm run test --> npm run check ; npm run test-only --> npm run test

9ee5270

Pull out traversal functions into separate module and constituent fun…

45efac6

…ctions.

Fix ForkedSequence for compressed sequences, and fix associated tests.

ef489e6

Bring tests up-to-date with new API.

c5df384

Bring template API into conformance with render API.

c674f4b

Put React.renderToString into production mode for more accurate bench…

dafae34

…marks.

divmain requested review from kenwheeler, aweary and ryan-roemer February 8, 2017 03:02

Revert concurrency setting in benchmark.

0794d54

ryan-roemer approved these changes Feb 8, 2017

View reviewed changes

Update README with new API.

0e5ca4d

divmain force-pushed the custom-async-primitive branch from dbd60ca to 0e5ca4d Compare February 8, 2017 06:04

Update lint task for Windows compatibiltiy.

febd8e2

divmain mentioned this pull request Feb 8, 2017

Render attrs correctly #19

Merged

1 task

aweary reviewed Feb 8, 2017

View reviewed changes

divmain mentioned this pull request Feb 8, 2017

SFCs are not rendered correctly when untranspiled #24

Closed

divmain added 2 commits February 8, 2017 14:12

Use more correct variable name.

15865c1

Merge branch 'master' into custom-async-primitive

ccfcb8f

divmain merged commit 74cdb27 into master Feb 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement custom async primitive #17

Implement custom async primitive #17

divmain commented Feb 8, 2017 •

edited

ryan-roemer left a comment

ryan-roemer Feb 8, 2017

ryan-roemer Feb 8, 2017

divmain Feb 8, 2017

ryan-roemer Feb 8, 2017

divmain Feb 8, 2017

aweary left a comment

aweary Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017 •

edited

aweary Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

divmain Feb 8, 2017

ryan-roemer Feb 8, 2017 •

edited

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017 •

edited

aweary Feb 8, 2017 •

edited

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

aweary Feb 8, 2017

divmain Feb 8, 2017

divmain commented Feb 8, 2017


		This function translates [Most.js](https://github.com/cujojs/most) streams to Node streams. You'll be able to pipe the output of these Node streams to an HTTP Response object or to disk.
		This function evalues a React VirtualDOM Element, and returns a Node stream. This stream will emit string segments of HTML as the DOM tree is asynchronously traversed and evaluated.


		However, you may want to change this number if your server is under heavy load. Possible values are the set of all positive integers. Lower numbers will be "more asynchronous" (shorter periods between I/O processing) and higher numbers will be "more synchronous" (higher performance).
		Note that the template composes both a stream of HTML text (`componentHtmlStream`) and a function that evaluates to the store's state - something you'll often want to do with SSR.

Implement custom async primitive #17

Implement custom async primitive #17

Conversation

divmain commented Feb 8, 2017 • edited

ryan-roemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-roemer Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain Feb 8, 2017 • edited

Choose a reason for hiding this comment

aweary Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divmain commented Feb 8, 2017

divmain commented Feb 8, 2017 •

edited

divmain Feb 8, 2017 •

edited

ryan-roemer Feb 8, 2017 •

edited

divmain Feb 8, 2017 •

edited

aweary Feb 8, 2017 •

edited