[PATCH]Read synchronously for larger files while concatenating #128
Conversation
lib/strategies/simple.js
Outdated
| }; | ||
|
|
||
| if (this._isContentLarge(content)) { | ||
| entry.content = ''; | ||
| entry.isFileContentLarge = true; |
There was a problem hiding this comment.
an alternative may be:
entry.isInMemory = true | false
lib/strategies/simple.js
Outdated
| }; | ||
|
|
||
| if (this._isContentLarge(content)) { | ||
| entry.content = ''; |
There was a problem hiding this comment.
lets do null or undefined here, that way we can disambiguate between empty content and no content.
lib/strategies/simple.js
Outdated
| @@ -76,10 +92,16 @@ class SimpleConcat { | |||
| } | |||
|
|
|||
| let entry = { | |||
There was a problem hiding this comment.
My early thoughts were something like:
class Entry {
constructor(path) {
this._content = undefined;
this.path = path;
}
get content() {
if (this._content !== undefined) { return this._content; }
let result = fs.readFileSync(this.path, 'UTF8');
if (result.length < TRESHOLD) { this._content = content; }
return result;
}
}Obviously I am open to other approaches. Just figured I would share.
There was a problem hiding this comment.
So, if I'm understanding this right, in this approach, we will not be updating the content in entry during add and update the first time. It would be computed just in result.
I think this might make the PR a lot simpler than what I have written and also get rid of the global baseDir(I really didn't want to do that).
There was a problem hiding this comment.
However, it seems like doing this might complicate writing unit tests as content need to be read from file instead of being assigned directly
There was a problem hiding this comment.
However, it seems like doing this might complicate writing unit tests as content need to be read from file instead of being assigned directly
Slightly more yes, but using node-fixturify should nearly mitigate this issue. (I think)
lib/strategies/simple.js
Outdated
| @@ -1,8 +1,13 @@ | |||
| 'use strict'; | |||
|
|
|||
| const findIndex = require('find-index'); | |||
| const fs = require('fs'); | |||
| let baseDir; | |||
There was a problem hiding this comment.
This will need to be addressed before we can land
akashdsouza
force-pushed
the
patch_retain_mitigate_memory_overhead
branch
from
5e48539
to
26f02e7
Compare
|
I have made changes as per the other approach. This does look cleaner. But, I'm not sure about the changes I have made to the unit tests. If these changes look good, I can squash the commits together |
test/strategies/simple-test.js
Outdated
| 'a.js': fileContent | ||
| }; | ||
| writeFixturifySync(obj); | ||
| concat.addFile(`${testDir}/a.js`, fileContent); |
There was a problem hiding this comment.
instead of ${testDir} in all this addFiles could we specify baseDir when creating SimpleConcat?
There was a problem hiding this comment.
Updated. I feel stupid for not doing something so obvious
these look good, i did leave 1 comment though |
|
Reading this made me realize I think there is some code cleanup -> #129 thoughts? |
akashdsouza
force-pushed
the
patch_retain_mitigate_memory_overhead
branch
from
26f02e7
to
ec6877c
Compare
It does look like it can be removed. I did a quick github search and there doesn't seem to be any weird usage. |
|
released as v3.5.0 🎉 |
|
I think this may be causing: #130 |
|
reverting this fixes #130 I'm going to roll this back. |
|
This PR depends on |
|
Although I haven't tested this yet, I think we can get around this by using |
This seems to work. However, I don't know enough about the internals of broccoli's node handling to confirm that this is the right approach. @stefanpenner Thoughts? |
|
In general you cannot make any assumptions about what |
|
Does this mean that I can use |
|
Yes |
First stab at #113. I'm not sure if this is the intended solution for the issue. I have started looking into broccoli plugins and this seemed like a good place to start.