Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Resource: getStream for empty string #249

Merged
merged 6 commits into from
Jun 24, 2020
Merged

[FIX] Resource: getStream for empty string #249

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ba41c0
[FIX] Resource: getStream for empty string
tobiasso85 Jun 17, 2020
f2f7c0e
[FIX] Resource: getStream for empty string
tobiasso85 Jun 17, 2020
7457e45
[FIX] Resource: getStream for empty string
tobiasso85 Jun 19, 2020
bfe587b
[FIX] Resource: getStream for empty string
tobiasso85 Jun 22, 2020
d740165
[FIX] Resource: getStream for empty string
tobiasso85 Jun 22, 2020
cf7d40b
[FIX] Resource: getStream for empty string
tobiasso85 Jun 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class Resource {
this._createStream = createStream || null;
this._stream = stream || null;
this._buffer = buffer || null;
if (string) {
if (typeof string === "string") {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tobiasso85 tobiasso85 Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@RandomByte RandomByte Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk, done

this._buffer = Buffer.from(string, "utf8");
}

Expand Down
45 changes: 35 additions & 10 deletions test/lib/Resource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@ function createBasicResource() {
return resource;
}

/**
* Reads a readable stream and resolves with its content
*
* @param {stream.Readable} readableStream readable stream
* @returns {Promise<string>} resolves with the read string
*/
const readStream = (readableStream) => {
return new Promise((resolve, reject) => {
let streamedResult = "";
readableStream.on("data", (chunk) => {
streamedResult += chunk;
});
readableStream.on("end", () => {
resolve(streamedResult);
});
readableStream.on("error", (err) => {
reject(err);
});
});
};

test("Resource: constructor with missing path parameter", (t) => {
t.throws(() => {
new Resource({});
Expand Down Expand Up @@ -62,20 +83,24 @@ test("Resource: getStream", async (t) => {
buffer: Buffer.from("Content")
});

return new Promise(function(resolve, reject) {
let streamedResult = "";
const readableStream = resource.getStream();
readableStream.on("data", (chunk) => {
streamedResult += chunk;
});
readableStream.on("end", () => {
resolve(streamedResult);
});
}).then((result) => {
return readStream(resource.getStream()).then((result) => {
t.is(result, "Content", "Stream has been read correctly");
});
});

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

Choose a reason for hiding this comment

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

Not necessary but doesn't hurt


const resource = new Resource({
path: "my/path/to/resource",
string: ""
});

return readStream(resource.getStream()).then((result) => {
t.is(result, "", "Stream has been read correctly for empty string");
});
});

test("Resource: getStream throwing an error", (t) => {
const resource = new Resource({
path: "my/path/to/resource"
Expand Down