Build LABjs string from source by gigabo · Pull Request #846 · redfin/react-server

gigabo · 2017-01-21T00:35:10Z

I'm going to start making substantive changes to the LABjs code we put at the top of every page. I want to have useful diffs and commit history for those changes, so as a baseline this PR replaces the pre-minified source string with the (mostly) vanilla source file.

A few benefits:

Can opt-in to viewing debug output with DEBUG_LAB=true react-server start
It's 210 bytes smaller (I'm going to make it smaller still in follow up PRs)
It's human-readable (and hackable)

A few drawbacks:

A dep on uglify
A few hundred millisecond startup penalty

Could minify during build to avoid those, but we'd lose the debug-ability which is really nice.

gigabo · 2017-01-21T00:35:40Z

I guess we could pre-build two copies (normal and debug) and select among them with an option... 🤔

That would actually let us turn on debugging at run-time which could be useful.

gigabo · 2017-01-21T00:44:01Z

Heh. 👹

doug-wade · 2017-01-21T00:48:52Z

🤔 iiuc, we're just checking in the source of lab.js to react server core, but is that really where it belongs? Maybe it makes sense to make it a separate npm package? Maybe it even makes sense to make it into an npm package that lives outside of the react server repo, maybe something like https://npmjs.com/package/labjs? It's under and MIT license, and it doesn't really seem like its something react server specific...

We could also add a git dependency to react-server/package.json

doug-wade · 2017-01-21T00:52:23Z

Actually, looks like there's plans to put it on npm

gigabo · 2017-01-21T00:52:36Z

Oh, good thought. I'm going to make some modifications for its use in React Server, so if anything it would probably have to be react-server-labjs or something like that. But really, I'm not sure that it would be useful externally? It's basically part of the React Server runtime in the browser.

doug-wade · 2017-01-21T00:53:49Z

Makes sense. I vote we pry it out into react-server-labjs (which also means you can postpone fixing the linting errors if you'd like 👹) and hope that we can take a dependency on the npm package when it comes out.

gigabo · 2017-01-21T00:55:15Z

hope that we can take a dependency on the npm package when it comes out.

We already can't. We would need getify/LABjs#81 to land for non-debug.

gigabo · 2017-01-21T00:56:07Z

But anyway once this (or whatever this turns into) lands I've got another PR coming on its heels that starts really hacking it up. 😈

drewpc · 2017-01-25T05:07:27Z

packages/react-server/core/util/LABString.js

+const path = require("path");
+const {readFileSync} = require("fs");
+
+var lab = readFileSync(path.join(__dirname, "LAB.src.js"), "utf-8");


Doesn't this mean that this module can't be used on the client side?

Yep, this module is only used in the render middleware. Its output is a string of JS that is put into an inline script tag.

drewpc · 2017-01-25T05:10:10Z

packages/react-server/core/util/LABString.js

+	lab = lab.replace(/\/\*!START_DEBUG(?:.|[\n\r])*?END_DEBUG\*\//g, "");
+
+	// After the debug code is removed just uglify.
+	lab = require("uglify-js").minify(lab, {fromString: true}).code;


We already run this whole package through Babel with Gulp, why not uglify it there and save the uglified version in target/(client|server)/util/LABString.js? What am I missing?

What am I missing?

The full extent of my laziness. 👹

As I think about it more I like @doug-wade's idea of pulling this out into a separate package. Going to manage it inside the React Server repo for now just for convenience. We'll be able to pull it out later using lerna export if we want to.

Future LABjs

The debug code gets totally removed in the minified build.

gigabo · 2017-01-27T00:12:21Z

I've updated this PR to put LAB into a dedicated package and move the minification to build time. 🍻

gigabo · 2017-01-27T00:16:45Z

Hard to believe flab wasn't taken. 😈

roblg · 2017-01-27T17:01:36Z

packages/flab/stringify.js

@@ -0,0 +1,9 @@
+console.log(
+	'module.exports = '+
+	require("fs").readFileSync("/dev/stdin", "utf-8")


I feel like I should know this, but... do we officially support windows? Seems like /dev/stdin wouldn't work there?

Ugh... this is only at build time. Do we need to support windows even for that? 😩

Great catch, @roblg. @gigabo use the memory-stream package for that! Examples:

mock socket: https://github.com/redfin/react-server/blob/master/packages/react-server/core/__tests__/reactMiddlewareSpec.js

mock fs: https://github.com/redfin/react-server/blob/master/packages/react-server-cli/src/__tests__/compileClient/compileClientSpec.js

Ignore the mockery stuff. The memory-stream package allows you to pass a stream object to a library and then call .toString() on what was written to the stream.

Oh cool thanks @drewpc.

I'm tempted to use get-stdin, which is a bit more targeted to what this little one-liner needs to do.

gigabo · 2017-01-27T20:01:11Z

Okay, got rid of the use of /dev/stdin. 👍

drewpc · 2017-01-27T21:08:22Z

While you're involved in this, it probably is a good idea to look into getify/LABjs#113. It might address #77 and #253 without much work from this end. Whoops... @doug-wade was on it and already mentioned this!

gigabo · 2017-01-27T22:29:16Z

Oh, nice. Thanks for the link.

doug-wade · 2017-01-30T00:11:48Z

packages/flab/README.md

+
+This is based on the awesome [LABjs](https://github.com/getify/LABjs) package.
+
+TODO: Flesh out this README. :grin:


doug-wade · 2017-01-30T00:12:12Z

packages/flab/package.json

@@ -0,0 +1,39 @@
+{
+  "name": "flab",


doug-wade · 2017-01-30T00:12:25Z

packages/flab/package.json

+    "loader"
+  ],
+  "author": "Bo Borgerson",
+  "license": "MIT",


LABjs is MIT. Can MIT be converted to Apache 2?

doug-wade · 2017-01-30T00:13:14Z

packages/flab/package.json

+  "bugs": {
+    "url": "https://github.com/redfin/react-server/issues"
+  },
+  "homepage": "https://github.com/redfin/react-server#readme",


out-of-scope: should we update all of the modules to point to react-server.io as their homepage?

Oh, maybe... 🤔

doug-wade

I dig it. Thanks @gigabo 🎉

gigabo · 2017-01-31T00:10:13Z

Onward! 👉

gigabo added the enhancement New functionality. label Jan 21, 2017

doug-wade approved these changes Jan 23, 2017

View reviewed changes

drewpc reviewed Jan 25, 2017

View reviewed changes

gigabo added 4 commits January 26, 2017 15:56

Add new package: flab

978837a

Future LABjs

Default debug to true

08b3372

The debug code gets totally removed in the minified build.

Add crossOrigin support back in

6ccd685

Add a basic README

0cbaab8

gigabo force-pushed the lab-hack branch from b4c3e15 to 0cbaab8 Compare January 27, 2017 00:10

roblg reviewed Jan 27, 2017

View reviewed changes

Portabled stdin slurp

6258686

drewpc modified the milestone: 0.6.1 Jan 27, 2017

doug-wade reviewed Jan 30, 2017

View reviewed changes

packages/flab/package.json

@@ -0,0 +1,39 @@

{

"name": "flab",

Copy link

Collaborator

doug-wade Jan 30, 2017

Choose a reason for hiding this comment

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

😆

doug-wade reviewed Jan 30, 2017

View reviewed changes

doug-wade approved these changes Jan 30, 2017

View reviewed changes

gigabo merged commit 77248a3 into redfin:master Jan 31, 2017


		This is based on the awesome [LABjs](https://github.com/getify/LABjs) package.

		TODO: Flesh out this README. :grin:

Conversation

gigabo commented Jan 21, 2017

Uh oh!

gigabo commented Jan 21, 2017

Uh oh!

gigabo commented Jan 21, 2017

Uh oh!

doug-wade commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doug-wade commented Jan 21, 2017

Uh oh!

gigabo commented Jan 21, 2017

Uh oh!

doug-wade commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gigabo commented Jan 21, 2017

Uh oh!

gigabo commented Jan 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigabo commented Jan 27, 2017

Uh oh!

gigabo commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gigabo commented Jan 27, 2017

Uh oh!

drewpc commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gigabo commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doug-wade left a comment

Choose a reason for hiding this comment

Uh oh!

gigabo commented Jan 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

doug-wade commented Jan 21, 2017 •

edited

Loading

doug-wade commented Jan 21, 2017 •

edited

Loading

drewpc commented Jan 27, 2017 •

edited

Loading