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

Jump to bottom

[FIX] Resource: Keep stats size up to date #253

Merged
merged 5 commits into from
Jun 26, 2020
Merged

[FIX] Resource: Keep stats size up to date #253

merged 5 commits into from
Jun 26, 2020
+95 −5

Conversation

tobiasso85
Copy link
Contributor

@tobiasso85 tobiasso85 commented Jun 25, 2020 •
edited
Loading

Ensure statInfo.size is always set when resource's content is modified.

@tobiasso85
[INTERNAL] Resource: Stats.size
31437e2 
Ensure size is always set when resource is modified.
@coveralls
Copy link

coveralls commented Jun 25, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.1%) to 87.229% when pulling 36b912e on resource-size into bc5eafb on master.

matz3
matz3 requested changes Jun 25, 2020
View reviewed changes
lib/Resource.js Outdated
@@ -76,6 +76,9 @@ class Resource {
if (typeof string === "string" || string instanceof String) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we rather call setString / setBuffer in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test/lib/Resource.js

t.is(resource.getStatInfo().size, 1337);
});

Copy link
Member

Choose a reason for hiding this comment

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

We should also add test cases when passing a real statInfo to the constructor (from fs.stat) and then changing the content. This is the use case when a resource is created by the FsAdapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tobiasso85
[INTERNAL] Resource: Stats.size
995e50f 
Use "real" Stat object in test
@tobiasso85
[INTERNAL] Resource: Stats.size
f3c43d7 
improve constructor logic
@tobiasso85 tobiasso85 requested a review from matz3 June 26, 2020 08:19
matz3
matz3 reviewed Jun 26, 2020
View reviewed changes
lib/Resource.js Outdated
@@ -74,7 +74,9 @@ class Resource {
this._stream = stream || null;
this._buffer = buffer || null;
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed

Copy link
Member

Choose a reason for hiding this comment

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

Done

lib/Resource.js Outdated
@@ -117,6 +119,7 @@ class Resource {
this._buffer = buffer;
this._contentDrained = false;
this._streamDrained = false;
this._setSize(Buffer.byteLength(this._buffer));
Copy link
Member

Choose a reason for hiding this comment

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

Buffer.byteLength just returns the byteLength from the buffer

Suggested change
this._setSize(Buffer.byteLength(this._buffer));
this._setSize(this._buffer.byteLength);

Copy link
Member

Choose a reason for hiding this comment

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

Done

@tobiasso85 tobiasso85 requested a review from matz3 June 26, 2020 09:26
@matz3
Copy link
Member

matz3 commented Jun 26, 2020

Maybe this is even a feature, to update the size when updating the content?

@tobiasso85 tobiasso85 changed the title [INTERNAL] Resource: Stats.size [FIX] Resource: Stats.size Jun 26, 2020
@tobiasso85 tobiasso85 changed the title [FIX] Resource: Stats.size [FIX] Resource: Stats.size keep up to date Jun 26, 2020
@tobiasso85 tobiasso85 changed the title [FIX] Resource: Stats.size keep up to date [FIX] Resource: Keep stats size up to date Jun 26, 2020
@tobiasso85 tobiasso85 merged commit 0ef976f into master Jun 26, 2020
@tobiasso85 tobiasso85 deleted the resource-size branch June 26, 2020 12:11
tobiasso85 added a commit to SAP/ui5-server that referenced this pull request Jun 26, 2020
@tobiasso85
[INTERNAL] serveIndex: fixed tests
d4e916d 
Made tests work together with
SAP/ui5-fs#253
@tobiasso85 tobiasso85 mentioned this pull request Jun 26, 2020
Merged
@RandomByte
Copy link
Member

As discussed today with @matz3 and @codeworrior: This does not work for resources where the content is updated using setStream(). For example processors/stringReplacer.js modifies the content in a streaming way.

A possible alternative solution could be to provide an asynchronous getSize() API which can transform a stream into a buffer and calculate the size if required.

tobiasso85 added a commit to SAP/ui5-server that referenced this pull request Jul 3, 2020
@tobiasso85
[INTERNAL] serveIndex: improved tests with regards to size (#335)
7208a26 
Changed tests to use the buffer size for the size display
when serving the index. Will work together with:
SAP/ui5-fs#253
@tobiasso85 tobiasso85 mentioned this pull request Jul 3, 2020
Merged
tobiasso85 added a commit that referenced this pull request Jul 6, 2020
@tobiasso85
[FIX] Resource#getSize: Retrieve Resource's size
7ef4d4e 
Stats.size was not set for a stream.
To ensure Resource's size can be retrieved for:
* stream
* string
* buffer
async method is introduced which uses #getBuffer to
ensure that size retrieval works for all content types.

followup of #253
@tobiasso85 tobiasso85 mentioned this pull request Jul 6, 2020
Merged
tobiasso85 added a commit that referenced this pull request Jul 13, 2020
@tobiasso85
[FIX] Resource#getSize: Retrieve Resource's size (#256)
9d2cc6c 
Stats.size was not set for a stream.
To ensure Resource's size can be retrieved for:
* stream
* string
* buffer
async method is introduced which uses #getBuffer to
ensure that size retrieval works for all content types.

followup of #253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matz3 matz3 matz3 approved these changes

@RandomByte RandomByte Awaiting requested review from RandomByte

@svbender svbender Awaiting requested review from svbender

@codeworrior codeworrior Awaiting requested review from codeworrior

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasso85 @coveralls @matz3 @RandomByte