Improve integration interface by mechanoid · Pull Request #1 · complate/complate-express

mechanoid · 2017-07-23T11:07:50Z

ships new complate-express interface (app.use(complate()) and res.render('my-component')
adds example test application
adds npm5 lock file
remove complate-jsx dependency from core project

complate-jsx is only needed as build dependency. Will add it to an example test project.

given a bundled set of complate components, exporting the complate documentRenderer ``` const bundle = require(path.resolve(__dirname, "dist", "bundle.js")); ``` we can hand this bundle to complate-express and register it as middleware ``` app.use(complate(bundle)); ``` Then we should be able to render arbitrary components with ``` app.get("/", (req, res) => { return res.render("site-index", { title: "Hello World!" }); }); ```

there is only a minimal manual test given ``` cd example npm install npm test curl http://localhost:5000 ~ Hello World! ```

this was inherited by complate-jsx before

FND

Thanks for this! I have a few notes - so to avoid getting bogged down by largely unrelated issues, I'd suggest we focus on getting the actual integration enhancement (i.e. #render) merged ASAP and discuss the remaining stuff elsewhere; WDYT?

FND · 2017-07-23T14:43:16Z

@@ -1,2 +1,3 @@
 /node_modules
 /.eslintcache
+/example/dist


... and yet you included example/dist/bundle.js - I assume that's unintentional?

Ah, I just noticed ba2c92c removes that file again - I wouldn't want to see it in the history at all though. (Don't worry about it for now; I'll probably rewrite some history anyway before merging this.)

exactly, and ok, i'm not that fan of rewriting the hist, but sure.

FND · 2017-07-23T14:52:59Z

+	return res.render("site-index", { title: "Hello World!" });
+});
+
+app.listen(5000);


While generally I'm a great advocate of providing minimal samples, in this case I'm wondering whether complate-sample-express isn't pretty much the same thing? This one feels a little like polluting the repo.

The thing I dislike on that complate-express-example is that it is not attached to the actual code, and it does not necessarily evolve with the given code. Providing it in the example section and even further making it a test artifact ensures that it stays in synch.

FND · 2017-07-23T14:53:56Z

+		</body>
+	</html>;
+});
+export default { hello: true };


This doesn't appear to be used anywhere?

debug output, I will remove it

FND · 2017-07-23T14:54:28Z

@@ -0,0 +1,11 @@
+// eslint-disable-next-line no-unused-vars


This shouldn't be necessary if ESLint knows about the JSX pragma.

hmm. In my editor i got warnings about that

You need this: https://github.com/complate/complate-jsx/blob/da474a45c2877fd365fb5593b957105bf80fcce1/samples/.eslintrc#L8

🤔 Perhaps we should package parts of that ESLint config so users just need to reference that:

extends: - fnd # optional; might as well be semistandard - complate

added the necessary options to the example project, but packaging sounds like a good thing. But it may be rather a thing like complate-jsx, right?

FND · 2017-07-23T14:54:53Z

 "use strict";

-let WritableStream = require("./writable_stream");
+const WritableStream = require("./writable_stream");


This still seems odd to me, as the community in my opinion strives for the latter. And especially while I don't want such variables to be reassigned later on let seems not right to me. In my opinion let gives a very clear signal, that I want to change the value after the first assignment. Using it by default removes that signal-function. But maybe its another discussion.

I'm with @mechanoid.

It's cute how y'all think this is a democracy, or that rational arguments might sway me.

Seriously though, let's just adhere to the surrounding code here and discuss stylistic concerns elsewhere (or, you know, not at all).

FND · 2017-07-23T15:00:00Z

-		response.end();
-	}
+	return (req, res, next) => {
+		res.render = (tag, params) => {


I like this approach (seems much simpler than Marko's solution), but I'm wary of overriding #render; I'd rather we add a method of our own instead.

tried to go with the standard mechanism of app.set('view engine') to make it work as designed, but then the first param has to be a filePath, that actually has to exist. So adding it via middleware-override is already a hack, so yes you may be right.

res.complate('some-component') should make it more clear?!

FND · 2017-07-23T15:01:24Z

-		this.reload();
-	}
+module.exports = bundle => {
+	const _render = bundle;


This alias seems redundant and indeed unnecessary?

Also: No. (This applies pretty much anywhere const is being used.)

I derived that from the code parts available before. Maybe bundle is not that good as param name so I will rename it

It also removes the redundant variable assignment

this allows a test setup (once it is provided) to test agains the latest stable libraries

this file should not be checked in, while it does not help to make this package more resilient. Tests (also manual smoke tests) should always run agains the latest stable dependencies

just rely on complate-jsx to handle the build pipeline aspects

add missing target to bundle build

This allows the complate rendering process to be up-2-date with re-created renderer-bundles once they changed.

FND · 2017-07-24T13:15:52Z

+module.exports = rendererPath => {
 	return (req, res, next) => {
+		const renderer = typeof rendererPath === "function" ?
+			rendererPath	:


naming and life is hard ;)

I was referring to \t:

🤣 sorry, my bad twin was at work, I guess. But funny, that eslint isn't even mentioning it

if cache is true, or app.enabled(view

youngbrioche · 2017-07-24T15:24:12Z

+
+## Quick Start Guide
+
+Install complate-express to your express application


s/Install to/Install in/?

youngbrioche · 2017-07-24T15:24:31Z

+Install complate-express to your express application
+
+```
+npm install complate-express


that's implicit since npm v5

Whut? Didn't know that.

the crucial part of npm5 ;)

youngbrioche · 2017-07-24T15:25:25Z

+    "express": "^4.15.3"
+  },
+  "devDependencies": {
+    "complate-jsx": "^0.7.1"


Why is this a devDependency?

complate-jsx is essentially a meta-package which abstracts the tools required for bundling and transpiling JSX modules (i.e. faucet-pipeline-js and babel-plugin-transform-react-jsx) - since complate templates are precompiled, those tools are not required at runtime

this definetly is a thing to be discussed, because often the same CI environment which runs the bundle process, is already running in production mode. But this is not important for that example application. And yes this dependency is not necessary once the bundle is created, so it should not be delivered to production surroundings.

yeah, I've been wondering about that CI issue as well ¯\_(ツ)_/¯

This is not a problem of this package — especially because it is just for the example part — but I recommend a layered build, in which dev-dependencies are installed, used and removed before released.

FND · 2017-07-24T16:04:40Z

+	}
+
+	return require(rendererPath);
+}


I struggled to grok what's really going on in this module. How about this:

module.exports = bundlePath => { bundlePath = path.resolve(bundlePath); let render = require(bundlePath); return (req, res, next) => { if(!res.app.enabled("view cache")) { // reload delete require.cache[bundlePath]; render = require(bundlePath); } res.complate = (tag, params) => { let stream = new WritableStream(res); render(stream, tag, params); res.end(); }; next(); }; };

hmm, while that is more readable yes, it looses some options I think a worth the hassle.

it does not allow to inject the bundle directly anymore, which decreases the possibilities of the user and decreases testing capabilities IMHO

cache configuration is missing. While I see the point, that the require.cache override is useful, it is quire intrusive, and it may be a good thing (at least for debugging purpose) to be able to skip it.

it requires the file twice if the cache is disabled

I guess I still haven't grokked it then:

I'm not sure that flexibility (and thus complexity) is warranted; YAGNI, also from a testing perspective

however, seems like an easy fix:

if(bundlePath.call) { render = bundlePath; } else { bundlePath = path.resolve(bundlePath); render = require(bundlePath); }

in your code, AFAICT, you're always passing { cache: true } into resolveRenderer, which is a private function - so I don't understand what the intent there is

well spotted, that's a bug then - but again, should be easy to fix?

maybe you are right there, will remove that part, to keep the interface easier, especially because of the behavioral change for function or path injection in terms of caching.

removed the options param from the interface by accident. Will add it again. I think a little bit control for that part seems still legit

as a side note. I would stay by 'rendererPath' instead of 'bundlePath' while that is the crucial part about that content, right? 'bundlePath' gives not really a hint about what the bundle is expected to contain (while that might be obvious)

I'm sorry to be obtuse, but I still don't understand the purpose of that cache parameter: Why not just rely on Express's "view cache" setting? So it seems to me this should suffice:

"use strict"; let WritableStream = require("./writable_stream"); // middleware to extend response object with `#complate` method module.exports = bundlePath => { bundlePath = path.resolve(bundlePath); return (req, res, next) => { if(!res.app.enabled("view cache")) { // ensure bundle is reloaded delete require.cache[bundlePath]; } let render = require(bundlePath); res.complate = (tag, params) => { let stream = new WritableStream(res); render(stream, tag, params); res.end(); }; next(); }; };

As for rendererPath vs bundlePath, I'd like to stick to the latter: It's not only an established convention elsewhere, but it actually seems more descriptive of what we're after there. Yes, the reader needs to be aware that this is where the render method comes from - but that's the entire point of this exercise, so it doesn't seem like an onerous expectation.

Hmm, maybe you are right about that, an additional config may be not necessary, right.

For the bundlePath I personally still don't like that, because it actually says nothing about its content or about what it has to expose. But I don't really care about that very much.

Will update that PR accordingly

updated the PR accordingly

I've merged this part as 7c845c6 (released as v0.9.0) because it moves things forward without getting bogged down by the remaining changes.

This commit also includes some adjustments WRT commit message, variable naming and conciseness (IMO anyway), plus it ensures bundlePath is absolute (I think? it's late and I'm tired... ).

Thank you for your patience; I'm somewhat overstretched at the moment.

FND · 2017-07-24T16:21:27Z

+
+## API
+
+#### complate(rendererPath: String|Renderer, options: Object)


you might want to mention that we're not using Express's view-engine mechanism because it doesn't support streaming - cf. http://markojs.com/docs/express/#skip-the-view-engine

Absolutely true, but maybe lets improve the README file afterwards?!

sure, fine by me - I just figured I'd mention it

removed the possibility to inject a required module in favor of keeping the interface simple. If it is necessary or useful in the future it can be extended easily

see complate#1 (comment)

FND · 2017-11-22T18:26:23Z

Sorry for leaving this unattended for so long. (July? WTF!)

Some of this has since been implemented (7c845c6 ff.), leaving the README and example application.

I'll see whether I can cherry-pick and update the former soon - no promises though, I'm a little short on cycles lately.

As mentioned elsewhere, I'd prefer to keep the latter in a separate repo - though I realize that comes with its own drawbacks, so let's discuss that when we get a chance.

@mechanoid

based on @mechanoid's efforts: #1

@mechanoid

based on @mechanoid's efforts: #1

@mechanoid

based on @mechanoid's efforts: #1

@mechanoid

based on @mechanoid's efforts: #1

Falk Hoppe added 6 commits July 23, 2017 12:04

remove complate-jsx as dependency

84c0bbf

complate-jsx is only needed as build dependency. Will add it to an example test project.

add npm5 package-lock file

9e8378e

add example express application for integration testing

68dd433

there is only a minimal manual test given ``` cd example npm install npm test curl http://localhost:5000 ~ Hello World! ```

add default-layout and fix bundling

ba2c92c

add missing linting dependency

0ba7ed4

this was inherited by complate-jsx before

FND reviewed Jul 23, 2017

View reviewed changes

Falk Hoppe added 8 commits July 23, 2017 20:32

remove debug code

7028b97

rename param say what input is expected.

fde9a6f

It also removes the redundant variable assignment

remove package-lock for lib

ab59b7c

this allows a test setup (once it is provided) to test agains the latest stable libraries

change render API to complate to not override standard functions

cdc75b9

ignore core lib package-lock file

81080a8

this file should not be checked in, while it does not help to make this package more resilient. Tests (also manual smoke tests) should always run agains the latest stable dependencies

simplify dev dependencies

cba1472

just rely on complate-jsx to handle the build pipeline aspects

fix faucet configuration

70e24d3

add missing target to bundle build

refresh the given renderer in dev-env

8b37b08

This allows the complate rendering process to be up-2-date with re-created renderer-bundles once they changed.

FND reviewed Jul 24, 2017

View reviewed changes

Falk Hoppe added 3 commits July 24, 2017 15:24

remove fnd-specific trolling

8d1ee0c

fix cache override

93b8dc7

if cache is true, or app.enabled(view

add minimal README

20358f7

youngbrioche reviewed Jul 24, 2017

View reviewed changes

FND reviewed Jul 24, 2017

View reviewed changes

Falk Hoppe added 7 commits July 24, 2017 19:13

fix eslint config for example project

0e803cd

fix missing options interface

54bc163

simplify rendererPath interface

8c7f4bc

removed the possibility to inject a required module in favor of keeping the interface simple. If it is necessary or useful in the future it can be extended easily

wording

d6ad4d9

rename rendererPath to bundlePath for reasons

229fe52

see complate#1 (comment)

remove redundant cache config

fe339b5

merged in build pipeline update

29bfce8

FND added a commit that referenced this pull request Nov 23, 2017

added README

f76dacf

based on @mechanoid's efforts: #1

FND added a commit that referenced this pull request Nov 23, 2017

added README

b11ffaa

based on @mechanoid's efforts: #1

FND added a commit that referenced this pull request Nov 23, 2017

added documentation

34889d0

based on @mechanoid's efforts: #1

FND mentioned this pull request Nov 23, 2017

added documentation #2

Closed

FND added a commit that referenced this pull request Nov 23, 2017

added documentation

0d4389a

based on @mechanoid's efforts: #1


		## Quick Start Guide

		Install complate-express to your express application


		## API

		#### complate(rendererPath: String\|Renderer, options: Object)

Conversation

mechanoid commented Jul 23, 2017

Uh oh!

FND left a comment

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!

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!

FND Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

FND Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

FND Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FND Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FND Jul 24, 2017 •

edited

Loading

FND Jul 24, 2017 •

edited

Loading

FND Jul 24, 2017 •

edited

Loading

FND Jul 24, 2017 •

edited

Loading

FND Jul 31, 2017 •

edited

Loading

FND Jul 31, 2017 •

edited

Loading