[FIX] Resource: getStream for empty string #249

tobiasso85 · 2020-06-17T08:08:26Z

If a resource does not have any content (empty string), the stream still can be retrieved.
Before the error Resource ${this._path} has no content
was thrown for the empty string.

Add additional test "getStream for empty file: correctly retrieved" which uses a buffer for an empty file for completeness.

CLAassistant · 2020-06-17T08:08:32Z

All committers have signed the CLA.

coveralls · 2020-06-17T08:16:03Z

Coverage increased (+0.03%) to 87.378% when pulling cf7d40b on resource-empty-string into b3eb22f on master.

RandomByte · 2020-06-18T16:05:47Z

test/lib/adapters/FileSystem_read.js

@@ -94,7 +94,7 @@ test("glob resources from application.a with directory exclude", async (t) => {
 	});

 	await readerWriter.byGlob("/!(pony,unicorn)/**").then(function(resources) {
-		t.deepEqual(resources.length, 2, "Found exactly two resource");
+		t.deepEqual(resources.length, 3, "Found exactly two resource");


Suggested change

t.deepEqual(resources.length, 3, "Found exactly two resource");

t.deepEqual(resources.length, 3, "Found exactly three resource");

RandomByte · 2020-06-18T16:10:01Z

test/lib/Resource.js

@@ -160,6 +185,12 @@ test("Resource: clone resource with stream", (t) => {
 	});
 });

+test("getStream for empty file: correctly retrieved", async (t) => {


This test is unrelated to your fix. It also passes without it. Why is it necessary? The commit message does not explain that.

added it for completeness, there was no test for a stream of an empty file. Will add it to the commit message.

will update message

Well I'm just wondering because half of the changes done in this PR are related to this test. And I'm not sure whether it tests any of our code.

In this test case, you pass in a stream (using the createStream callback) and retrieve it using getStream. The Resource implementation never handles the streams content. So it doesn't matter whether it's empty or not. Maybe I'm missing something but to me this test seems pretty useless?

RandomByte · 2020-06-18T16:13:04Z

Also, this should be a [FIX] so that it is visible in the changelog

If a resource does not have any content (empty string). The stream still can be retrieved. Before the error `Resource ${this._path} has no content` was thrown for the empty string.

Improve documentation and variable naming

Adjust wording

remove test

RandomByte · 2020-06-22T11:49:01Z

test/lib/Resource.js

@@ -4,10 +4,10 @@ const fs = require("fs");
 const path = require("path");
 const Resource = require("../../lib/Resource");

-function createBasicResource() {
-	const fsPath = path.join("test", "fixtures", "application.a", "webapp", "index.html");
+function createBasicResource(fileName = "index.html") {


This change is not necessary anymore

RandomByte · 2020-06-22T11:49:33Z

test/lib/adapters/FileSystem_read.js

@@ -94,7 +94,7 @@ test("glob resources from application.a with directory exclude", async (t) => {
 	});

 	await readerWriter.byGlob("/!(pony,unicorn)/**").then(function(resources) {
-		t.deepEqual(resources.length, 2, "Found exactly two resource");
+		t.deepEqual(resources.length, 3, "Found exactly three resource");


test/fixtures/application.a/webapp/empty.js can be removed again and this change reverted

removed unnecessary test content

RandomByte · 2020-06-22T14:51:20Z

lib/Resource.js

@@ -73,7 +73,7 @@ class Resource {
 		this._createStream = createStream || null;
 		this._stream = stream || null;
 		this._buffer = buffer || null;
-		if (string) {
+		if (typeof string === "string") {


Should we also check for string instanceof String to catch cases where a new String("bla") object is passed?

I would either use then both in combination:
typeof and instanceof

let simpleStr = 'This is a simple string' let myString = new String() let newStr = new String('String created with constructor') let owString = String("asd") simpleStr instanceof String // returns false, string literal is not an object myString instanceof String // returns true newStr instanceof String // returns true owString instanceof String // returns false (newStr + "a") instanceof String // returns false newStr.toString() instanceof String // returns false

source + modified:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof

or leave typeof because that is rare that a string is constructed using the constructor with new.

Also see
https://google.github.io/styleguide/jsguide.html#disallowed-features-wrapper-objects

Yes, I would add that as an additional check as well. Just like here: https://stackoverflow.com/a/9436948

or leave typeof because that is rare that a string is constructed using the constructor with new.

Yes it's rare, but if somebody does it we would still create the resource but without any content. That might cause issues that are not easy to understand. We should add this check to prevent this from happening.

add check for string instance

RandomByte

LGTM

Please squash and merge, otherwise we will have 6 new entries in the changelog instead of one

RandomByte · 2020-06-23T13:31:33Z

test/lib/Resource.js

 		t.is(result, "Content", "Stream has been read correctly");
 	});
 });

+test("Resource: getStream for empty string", async (t) => {
+	t.plan(1);


Not necessary but doesn't hurt

tobiasso85 requested review from RandomByte and matz3 June 17, 2020 08:21

RandomByte requested changes Jun 18, 2020

View reviewed changes

tobiasso85 requested a review from RandomByte June 19, 2020 12:52

tobiasso85 changed the title ~~[INTERNAL] Resource: getStream for empty string~~ [FIX] Resource: getStream for empty string Jun 22, 2020

tobiasso85 added 3 commits June 22, 2020 09:26

[FIX] Resource: getStream for empty string

9ba41c0

If a resource does not have any content (empty string). The stream still can be retrieved. Before the error `Resource ${this._path} has no content` was thrown for the empty string.

[FIX] Resource: getStream for empty string

f2f7c0e

Improve documentation and variable naming

[FIX] Resource: getStream for empty string

7457e45

Adjust wording

tobiasso85 force-pushed the resource-empty-string branch from d10cecb to 7457e45 Compare June 22, 2020 07:27

[FIX] Resource: getStream for empty string

bfe587b

remove test

RandomByte requested changes Jun 22, 2020

View reviewed changes

[FIX] Resource: getStream for empty string

d740165

removed unnecessary test content

tobiasso85 requested a review from RandomByte June 22, 2020 13:30

RandomByte reviewed Jun 22, 2020

View reviewed changes

[FIX] Resource: getStream for empty string

cf7d40b

add check for string instance

RandomByte approved these changes Jun 23, 2020

View reviewed changes

tobiasso85 merged commit bc5eafb into master Jun 24, 2020

tobiasso85 deleted the resource-empty-string branch June 24, 2020 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Resource: getStream for empty string #249

[FIX] Resource: getStream for empty string #249

tobiasso85 commented Jun 17, 2020 •

edited

Loading

CLAassistant commented Jun 17, 2020 •

edited

Loading

coveralls commented Jun 17, 2020 •

edited

Loading

RandomByte Jun 18, 2020

tobiasso85 Jun 19, 2020

tobiasso85 Jun 19, 2020

RandomByte Jun 18, 2020

tobiasso85 Jun 19, 2020

tobiasso85 Jun 19, 2020

RandomByte Jun 22, 2020

tobiasso85 Jun 22, 2020

RandomByte commented Jun 18, 2020

RandomByte Jun 22, 2020

tobiasso85 Jun 22, 2020

RandomByte Jun 22, 2020

tobiasso85 Jun 22, 2020

RandomByte Jun 22, 2020

tobiasso85 Jun 23, 2020 •

edited

Loading

RandomByte Jun 23, 2020 •

edited

Loading

tobiasso85 Jun 23, 2020

RandomByte left a comment

RandomByte Jun 23, 2020

	t.deepEqual(resources.length, 3, "Found exactly two resource");
	t.deepEqual(resources.length, 3, "Found exactly three resource");

[FIX] Resource: getStream for empty string #249

[FIX] Resource: getStream for empty string #249

Conversation

tobiasso85 commented Jun 17, 2020 • edited Loading

CLAassistant commented Jun 17, 2020 • edited Loading

coveralls commented Jun 17, 2020 • edited Loading

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

RandomByte commented Jun 18, 2020

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

tobiasso85 Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

RandomByte Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasso85 commented Jun 17, 2020 •

edited

Loading

CLAassistant commented Jun 17, 2020 •

edited

Loading

coveralls commented Jun 17, 2020 •

edited

Loading

tobiasso85 Jun 23, 2020 •

edited

Loading

RandomByte Jun 23, 2020 •

edited

Loading