Initial code PR #2

CameronDiver · 2017-02-13T13:16:39Z

Last PR was closed by GH due to a force push (sorry!) so this PR is actually the previous, with comments addressed.

Last PR: #1

Page- · 2017-02-14T01:37:17Z

There doesn't appear to be a package.json

Page- · 2017-02-14T01:38:27Z

Ok, it appears that the package.json with all dependencies included is in the initial commit - it would be nicer to have the dependencies as part of this PR

Page- · 2017-02-14T01:40:13Z

src/index.ts

+		const pack = tar.pack()
+
+		return Promise.all(
+			Promise.resolve(fs.readdir(dirPath))


It's probably nicer to use Promise.promisify for this once rather than having to convert the promise every time we call an mz/fs function

Page- · 2017-02-14T01:42:20Z

src/index.ts

+	public buildDir(dirPath: string, buildOpts: Object, hooks: Plugin.IBuildHooks): Promise<NodeJS.ReadableStream> {
+		const pack = tar.pack()
+
+		return Promise.all(


This looks like you're doing a Promise.all over a single promise argument? I'm guessing this is either a leftover or something got dropped by accident

Good catch, that was from when I was mapping over the synchronous fs functions.

Page- · 2017-02-14T01:44:21Z

src/index.ts

+
+		this.layers = []
+
+		if (hooks == null) {


I think you can add the default to the func declaration, eg

public createBuildStream(buildOpts: Object, hooks: Plugin.IBuildHooks = {}): NodeJS.ReadWriteStream {

Page- · 2017-02-14T01:47:14Z

src/index.ts

+		if (hook in hooks) {
+			// Spread the arguments onto the callback function
+			const fn = hooks[hook]
+			if (fn !== undefined) {


Using _.isFunction(fn) probably makes more sense

Page- · 2017-02-14T01:47:37Z

src/index.ts

+	 * @returns {any} The return value of the function, or nothing if the
+	 * function does not exist or does not provide a return value
+	 */
+	private callHook = (hooks: Plugin.IBuildHooks, hook: string, ...args: any[]) : any => {


Is the = meant to be in here?

CameronDiver · 2017-02-14T08:54:53Z

Yeah with the package.json I wasn't sure because I wanted it there for @hedss' versionist to merge this PR but wasn't sure going through another PR made any sense. Both ways felt a bit janky tbh.

hedss · 2017-02-20T13:05:58Z

lib/plugin.d.ts

+ * Because of this the minimum recommended registered hooks are buildSuccess and buildFailure,
+ * but this is not enforced, or required.
+ */
+export interface IBuildHooks {


As @pimterry has pointed out in another review (which I'm really sorry, I can't find), the denoted way to define interfaces is without the I. And in fact, MS recommend you don't:
microsoft/TypeScript-Handbook#121

Personally, I don't mind. But if we prefix I for interfaces, then are we going to use shortened HN elsewhere? I think this probably needs to be specific in the TS spec. (though for some reason I thought I had put it in there).

And sorry this comment is so late!

Ah, I shouldn't have been listening to the default tslint config. I don't like to prefix them with I anyway :).

hedss

TSwise, I think it looks ok (apart from my one comment, which applies in quite a few places).
@pimterry will probably have more. :)

hedss · 2017-02-22T17:15:02Z

src/example/project-type.ts

+	private templateContent: string = ''
+
+	public provideEntry(stream: NodeJS.ReadableStream, header: any): boolean {
+		// Check if this is a file we need to be able to generate the dockerfile


I think the consensus was we were going to allow implicit typing where possible, so as this method always returns a boolean, you won't need it declared in the signature.
Same is true elsewhere.

Previous discussion: balena-io-modules/balena-procbots#5 (comment). I think the conclusion is:

Definitely skip it (do implicit typing) if it's very obvious (let x = new X())

Probably type things explicitly otherwise, to taste.

I think method return types are one case though where explicit types are particularly often useful though, and we should be including them. If you explicitly specify this return type, TypeScript checks every path through the method, and guarantees they all always return booleans. Easy to end up with an accidental implicit return or the wrong return type otherwise.

pimterry · 2017-02-22T17:27:13Z

src/example/project-type.ts

+	private templateContent: string = ''
+
+	public provideEntry(stream: NodeJS.ReadableStream, header: any): boolean {
+		// Check if this is a file we need to be able to generate the dockerfile


Previous discussion: balena-io-modules/balena-procbots#5 (comment). I think the conclusion is:

Definitely skip it (do implicit typing) if it's very obvious (let x = new X())

Probably type things explicitly otherwise, to taste.

I think method return types are one case though where explicit types are particularly often useful though, and we should be including them. If you explicitly specify this return type, TypeScript checks every path through the method, and guarantees they all always return booleans. Easy to end up with an accidental implicit return or the wrong return type otherwise.

pimterry · 2017-02-22T17:28:34Z

src/example/app.ts

+		console.log(`Image Id: ${imageId}`)
+		console.log(`Image layers: ${JSON.stringify(layers, null, '  ')}`)
+	},
+	buildFailure: (error: Error) : void => {


Nit: spacing for return types isn't consistent. Compare ) : void here with ): void above. Tiny point, and I don't really mind which way we go, but it'd be nice to pick a single style.

That's completely by accident :/ I was going for ): type every time. The example code is being taken out in favour of a nice README anyway, but definitely need to watch out for that.

hedss

Given the conversation, LGTM apart from the other two points @pimterry raises.

pimterry · 2017-02-22T17:40:07Z

src/plugin.d.ts

+	/**
+	 * Index signature
+	 */
+	[key: string]: ((...args: any[]) => any) | undefined


What's this index signature for? Is it because you want to support arbitrary other new hooks, and you don't know what they are somehow, or is it just because callHook won't compile otherwise?

If the former, fine, if the latter, you should be able to do this more tidily and statically safely with constant string literals. Here's a typescript playground that sort of demonstrates the idea.

pimterry · 2017-02-22T17:41:18Z

src/tests/tests.ts

+
+// In general we don't want output, until we do.
+// call with `env DISPLAY_TEST_OUTPUT=1 npm test` to display output
+const dockerPath = process.env.DOCKER_PATH ? process.env.DOCKER_PATH : '/var/run/docker.sock'


Nit: this could just be process.env.DOCKER_PATH || '/var/run/docker.sock'

Page-

Can you move the tests from /src/tests to /tests please, I expect /src to only contain actual source code for the module (unless the /src/tests is a typescript convention, @pimterry?)

Page- · 2017-02-28T07:22:04Z

src/utils.ts

+}
+
+const extractArrowMessage = (message: string): string | undefined => {
+	const arrowTest = /^\s*-+>\s*(.+)/i


There's no need for the i - there's nothing that is affected by case-sensitivity

Page- · 2017-02-28T07:25:19Z

src/builder.ts

+				return fn.apply(null, args)
+			}
+		}
+		return undefined


Here you explicity return undefined, but in other places I see you just return, it would be nice to be consistent (personally I prefer return) - is this covered by the TS style guide?

Good catch, yeah I'll need to be consistent with this. The style guide unfortunately isn't ready but it was becoming a blocker. I don't mind going back through and doing a style overhaul, but I want to get the main semantics done.

Page- · 2017-02-28T07:28:09Z

src/builder.ts

+			const relPath = path.join(dirPath, file)
+			return Promise.all([file, fs.stat(relPath), fs.readFile(relPath)])
+		})
+		.map((fileInfo: any[]) => {


We know the types for this will be [string, fs.Stats, Buffer] - is there any reason we can't be explicit?

Page- · 2017-02-28T07:32:38Z

src/builder.ts

+		const pack = tar.pack()
+
+		return this.readdirBluebird(dirPath)
+		.map((file: string) => {


Can you indent this to show more clearly that it's nested as part of the return statement, rather than chaining directly from it eg with the current indentation I expect return(something).map().map() semantics with a nested chain I would expect return (something.map().map()...) semantics - maybe it's just me but it does feel like a decent styling

A clearer example might be that the current version feels to me a bit like

fn(this.readdirBluebird(dirPath) .map((file: string) => { }) .map() )

Page- · 2017-02-28T07:36:09Z

src/builder.ts

+	 */
+	public createBuildStream(buildOpts: Object, hooks: Plugin.BuildHooks = {}): NodeJS.ReadWriteStream {
+
+		const instance = this


I think it's common to use const self = this for self-references

Page- · 2017-02-28T07:37:32Z

src/builder.ts

+	constructor(dockerOpts: Dockerode.DockerOptions) {
+		this.docker = new Dockerode(dockerOpts)
+
+		// Promisify individual functions, to keep type information


Is this meant to be // TODO: ?

Ah that's a classic case of comments not keeping up to date with code, nice catch. I actually struggled with getting promisification and TS to work nicely, and this seemed to be the best trade-off.

Page- · 2017-02-28T07:40:17Z

lib/builder.d.ts

@@ -0,0 +1,64 @@
+/// <reference types="node" />


Is this file actually needed? Wouldn't TS dependants just point to the .ts source directly in order to have types direct from the source code (and also be able to use "go to definition", etc), cc @pimterry

I'll prefix this comment by saying I've been piecing together the way to package up a TS library from various examples around the web so I'm not 100% that this is the way it should be done.

That said, it seems like the advantage to doing it this way is that the library can be consumed from plain JS, (and therefore all derivatives) and then if the library is being consumed from TS, the compiler will pull in the .d.ts files in addition. It just means AFAIK that changes aren't needed to allow it to work out-of-the-box in both JS and TS.

I think you can use:

"main": "lib/index.js", "types": "src/index.ts",

and it should work as I described

Oh, nice! :)

Page- · 2017-02-28T07:41:27Z

build.sh

@@ -0,0 +1,14 @@
+#!/bin/bash


I'd like to avoid bash-only building, it makes this module a bit trickier to use on windows

Yeah I'm not keen on that myself, definitely going to change it to something like a gruntfile, I've just never looked into it.

CameronDiver · 2017-02-28T11:36:43Z

The ./src/tests vs ./tests stemmed from the fact that tsc does not like any source files to be below the source root in tsconfig.json, which in this case is ./src. An alternative to the way that I've done it is to add ./tests/ to the exclude section of tsconfig.json and then only build the tests when testing. I have no strong opinion, so if you think that's better then I'll do that. @Page-

Page- · 2017-03-01T00:06:26Z

package.json

-    "test": "echo \"Error: no test specified\" && exit 1"
+    "start": "",
+    "build": "./build.sh",
+    "test": "node_modules/.bin/mocha -r ts-node/register src/tests/tests.ts"


Can you change node_modules/.bin/mocha to just mocha, it will be automatically pointed to the correct node_modules path by npm

Page- · 2017-03-01T00:08:50Z

There's definitely ways to have the test dir at the top level and not have tsc complain (I did that in typed-error, and never saw any error) - I'm guessing it's something to do with this particular set of tsconfig.json options

Page-

Do the js versions of the tests need to be committed? With the typescript loader I think the tests should be able to be run directly without having to write the js to disk

Page- · 2017-03-03T01:25:05Z

tsconfig.json

@@ -12,6 +11,7 @@
  },
  "exclude": [
    "node_modules",
-    "lib"
+    "lib",
+		"tests"


Wonky indentation here

Page- · 2017-03-03T01:26:02Z

package.json

  "description": "A containerised builder which interacts with the docker remote API to perform builds.",
  "main": "lib/index.js",
-  "types": "lib/index.d.ts",
+  "types": "srcr/index.ts",


srcr -> src

Wow that's really weird, I wonder why the imported types were working from an external module... strange. Anyway, nice catch!

* Created plugin based architecture which modules users can hook into * Add tests * Add gulpfile * Add README * Add deps to package.json Signed-off-by: Cameron Diver <cameron@resin.io>

CameronDiver requested review from Page-, petrosagg, pimterry, hedss, dfunckt and lekkas February 13, 2017 13:16

Page- suggested changes Feb 14, 2017

View reviewed changes

hedss reviewed Feb 20, 2017

View reviewed changes

CameronDiver force-pushed the develop branch from 5655d56 to a12a325 Compare February 21, 2017 11:55

hedss suggested changes Feb 22, 2017

View reviewed changes

pimterry reviewed Feb 22, 2017

View reviewed changes

hedss approved these changes Feb 22, 2017

View reviewed changes

pimterry reviewed Feb 22, 2017

View reviewed changes

CameronDiver force-pushed the develop branch from b826a0b to 99b79b7 Compare February 27, 2017 14:47

Page- suggested changes Feb 28, 2017

View reviewed changes

Page- reviewed Mar 1, 2017

View reviewed changes

Page- suggested changes Mar 3, 2017

View reviewed changes

CameronDiver force-pushed the develop branch 2 times, most recently from fc7158d to 365c437 Compare March 3, 2017 11:58

Page- approved these changes Mar 6, 2017

View reviewed changes

Initial code commit

e38e74d

* Created plugin based architecture which modules users can hook into * Add tests * Add gulpfile * Add README * Add deps to package.json Signed-off-by: Cameron Diver <cameron@resin.io>

CameronDiver force-pushed the develop branch from 365c437 to e38e74d Compare March 7, 2017 10:11

CameronDiver merged commit f8a33a1 into master Mar 7, 2017

pdcastro deleted the develop branch March 13, 2019 00:59

Initial code PR #2

Initial code PR #2

Conversation

CameronDiver commented Feb 13, 2017 • edited Loading

Page- commented Feb 14, 2017

Page- commented Feb 14, 2017

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

CameronDiver commented Feb 14, 2017

hedss Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hedss 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

hedss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Page- 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

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

CameronDiver commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Page- commented Mar 1, 2017

Page- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CameronDiver Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

CameronDiver commented Feb 13, 2017 •

edited

Loading

hedss Feb 20, 2017 •

edited

Loading

CameronDiver commented Feb 28, 2017 •

edited

Loading

CameronDiver Mar 3, 2017 •

edited

Loading