Add stream event listener argument to resolveInput() #29

pdcastro · 2019-02-04T13:24:54Z

Resolves: #28
Change-type: major
Signed-off-by: Paulo Castro paulo@balena.io

pdcastro

See issue #28 for a description of what this PR intends to achieve.

pdcastro · 2019-02-04T13:30:16Z

package-lock.json

@@ -0,0 +1,4009 @@
+{


To recap / confirm the recommended practice, it is desirable to add package-lock files to libraries (such as resin-bundle-resolve) but not to "apps" installed by users (such as balena-cli); is this right?

It's the opposite @pdcastro , we definitely want them in apps, but we don't care as much in libraries. That being said, having them in libraries too can be useful to ensure a consistent CI build.

But there was a good reason why balena-cli does not commit package-lock.json (it is in .gitignore), while the libs imported by the CLI can/should have package-lock.json files. @thgreasi, you might remember what the reason was... I think it was to do with we not having control over the customer's environment -- like, package-lock.json was ignored (?) in some cases or platforms (?), so we wanted the CI to try installing the latest versions of whatever is in package.json -- but I am not able to fully justify it to myself. :-)

I believe @Page- also has some theories on this too.

pdcastro · 2019-02-04T13:32:09Z

src/index.ts

 	dockerfile?: string,
-): Readable {


tar.Pack implements the Readable interface, so this should be a "safe" change. Some callers of resolveInput were doing explicit cast from Pack to Readable, and this change avoids the need for that.

It used to be that the typings were incompatible iirc, but if it's all working well now then I agree this is worth it.

pdcastro · 2019-02-04T13:35:10Z

tsconfig.json

@@ -7,7 +7,7 @@
 		"declaration": true,
 		"pretty": true,
 		"removeComments": true,
+		"skipLibCheck": true,


Needed because tsc was failing in type checks for the strict-event-emitter-types package. The skipLibCheck flag is used in many other balena packages.

Yep, should definitely be included.

CameronDiver · 2019-02-04T13:57:32Z

src/index.ts

 	const extract = tar.extract();
 	const pack = tar.pack();
+	for (const event of Object.keys(resolveListeners) as Array<


I think you should be able to do:

Suggested change

for (const event of Object.keys(resolveListeners) as Array<

for (const event in resolveListeners)

If I write it like that (in and no type cast), then on the following line I get a compiler error:

src/index.ts:75:21 - error TS7017: Element implicitly has an 'any' type because type 'ResolveListeners' has no index signature. 75 const listeners = resolveListeners[event] || [];

Also, the in operator iterates over all of the object's prototype chain's properties (which in some cases results in the linter warning that the for loop needs an if test for "own properties"), while Object.keys() returns the keys for the object's own properties only.

CameronDiver · 2019-02-04T14:00:12Z

src/index.ts

+		const listeners = resolveListeners[event] || [];
+		for (const listener of listeners) {
+			pack.on(event, listener);
+		}


We already have lodash imported here so this could be

_.each(resolveListeners, (cbs, event) => _.each(cbs, cb => pack.on(event, cb)))

If I do it like that, then the compiler errors:

src/index.ts:83:36 - error TS2345: Argument of type 'number | ((chunk: string | Buffer) => void) | ((e: Error) => void) | ((...items: (() => void)[]) ...' is not assignable to parameter of type 'Function'. Type 'number' is not assignable to type 'Function'. 83 _.each(cbs, cb => pack.on(event, cb)),

Also:

npm run prettify expands it to a minimum of 3 lines of code, so the one-liner temptation is partially spoiled.

I find the nested callbacks slightly harder to read than the nested for loops (one needs to be familiar with lodash's callback arguments...).

I can however reduce one line of code by removing the const listeners variable declaration:

for (const event of Object.keys(resolveListeners) as Array< keyof ResolveListeners >) { for (const listener of resolveListeners[event] || []) { pack.on(event, listener); } }

pdcastro · 2019-02-04T16:22:07Z

src/index.ts

 export function resolveInput(
 	bundle: Bundle,
 	resolvers: Resolver[],
+	resolveListeners: ResolveListeners,


This added argument is the reason for a major version bump.

Why not put it last and make it optional?

Yes, I thought about making it optional, it could be done. But then I considered that not providing this argument and thus potentially ignoring error events is "almost certainly a bug" and the function interface should discourage it. Adding this argument is the main purpose of this PR. Before this change, there wasn't a foolproof way of providing an error handler for the stream events. Making this arg optional would be like making the improvement optional...

pdcastro · 2019-02-07T12:02:40Z

src/index.ts

+		});
+	});
+	entry.stream.pipe(newEntryStream);
+	await newEntryStreamPromise;


The ~10 lines above (newEntryStreamPromise) were necessary to avoid a tar-stream limitation by which it is only possible to pipe to one pack.entry at a time -- so it's necessary to wait for the 'finish' event on one entry before moving on to the next entry. Some resin-builder tests were failing with the tar-stream error "Error: already piping an entry". More details here: mafintosh/tar-stream#24 (comment)

pdcastro · 2019-02-07T12:04:55Z

src/index.ts

+					)) || specifiedFileResolver;
+			} catch (error) {
+				console.log('resin-bundle-resolve extract entry -> error', error);
+				pack.emit('error', error);


Some errors thrown in stream listeners were not being caught, so I added the try-catch above and "forwarded" the errors through pack.emit('error').

pdcastro · 2019-02-07T12:05:43Z

src/index.ts

+				'resin-bundle-resolve extract on finish calling pack.finalize()',
+			);
+			pack.finalize();
+		}


Some errors thrown in stream listeners were not being caught and pack.finalize() not called, so I added the try-catch-finally above and "forwarded" the errors through pack.emit('error').

(semver 'major' change because this commit makes the resolveInput function incompatible with previous major versions by adding a new required argument.) Change-type: major Signed-off-by: Paulo Castro <paulo@balena.io>

pdcastro commented Feb 4, 2019

View reviewed changes

pdcastro requested a review from CameronDiver February 4, 2019 13:35

CameronDiver requested review from zvin, dfunckt and Page- February 4, 2019 13:55

CameronDiver reviewed Feb 4, 2019

View reviewed changes

pdcastro requested a review from thgreasi February 4, 2019 14:05

pdcastro mentioned this pull request Feb 4, 2019

Add event listener argument to performResolution() (dockerfile selection) balena-io-modules/balena-multibuild#41

Merged

pdcastro force-pushed the 28-add-resolveInput-listeners branch from 300ea2f to e3df60b Compare February 4, 2019 16:10

pdcastro commented Feb 4, 2019

View reviewed changes

CameronDiver approved these changes Feb 4, 2019

View reviewed changes

pdcastro force-pushed the 28-add-resolveInput-listeners branch from e3df60b to a61834b Compare February 7, 2019 11:57

pdcastro commented Feb 7, 2019

View reviewed changes

pdcastro force-pushed the 28-add-resolveInput-listeners branch from a61834b to f5c2d7b Compare February 8, 2019 14:07

pdcastro force-pushed the 28-add-resolveInput-listeners branch 2 times, most recently from 0142f33 to f8de286 Compare February 17, 2019 13:39

Added ability to specify resolve listeners for stream events

811ceba

(semver 'major' change because this commit makes the resolveInput function incompatible with previous major versions by adding a new required argument.) Change-type: major Signed-off-by: Paulo Castro <paulo@balena.io>

pdcastro force-pushed the 28-add-resolveInput-listeners branch from f8de286 to 811ceba Compare February 18, 2019 16:24

pdcastro merged commit e625223 into master Feb 18, 2019

pdcastro deleted the 28-add-resolveInput-listeners branch February 18, 2019 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stream event listener argument to resolveInput() #29

Add stream event listener argument to resolveInput() #29

pdcastro commented Feb 4, 2019 •

edited

pdcastro left a comment

pdcastro Feb 4, 2019

CameronDiver Feb 4, 2019

pdcastro Feb 4, 2019

CameronDiver Feb 4, 2019

pdcastro Feb 4, 2019

CameronDiver Feb 4, 2019

pdcastro Feb 4, 2019

CameronDiver Feb 4, 2019

CameronDiver Feb 4, 2019

pdcastro Feb 4, 2019

CameronDiver Feb 4, 2019

pdcastro Feb 4, 2019 •

edited

pdcastro Feb 4, 2019

dfunckt Feb 4, 2019

pdcastro Feb 4, 2019 •

edited

pdcastro Feb 7, 2019 •

edited

pdcastro Feb 7, 2019

pdcastro Feb 7, 2019

	for (const event of Object.keys(resolveListeners) as Array<
	for (const event in resolveListeners)

Add stream event listener argument to resolveInput() #29

Add stream event listener argument to resolveInput() #29

Conversation

pdcastro commented Feb 4, 2019 • edited

pdcastro 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

pdcastro Feb 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdcastro Feb 4, 2019 • edited

Choose a reason for hiding this comment

pdcastro Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdcastro commented Feb 4, 2019 •

edited

pdcastro Feb 4, 2019 •

edited

pdcastro Feb 4, 2019 •

edited

pdcastro Feb 7, 2019 •

edited