#1393: Only use public API of 'compiler.inputFileSystem' #1437
Conversation
DorianGrey
force-pushed
the
1393-use-public-filesystem-api
branch
from
38f009d to
c85cce2
Compare
|
Thanks for the contribution, @DorianGrey! I'm generally on board with this change, but would like to CC: in @goldhand (who wrote the original I don't want to block on either of those folks as I'm not sure that they'll have time to provide feedback, so I'll also run a few quick sanity checks. (Longer-term, I think it would make sense to specifically add some tests that cover alternative filesystems; see #1456) |
| @@ -28,7 +28,7 @@ | |||
| */ | |||
| function readFileWrapper(readFileFn, filePath) { | |||
| return new Promise((resolve, reject) => { | |||
| readFileFn(filePath, 'utf8', (error, data) => { | |||
There was a problem hiding this comment.
Would the utf8 encoding option cause any problem? This seems like the only different in calling the readFile API.
There was a problem hiding this comment.
Yes - it causes a runtime error, since the method from the public API only expects two parameters, the file path and the callback. See the API of the CachedInputFileSystem for an example: https://github.com/webpack/enhanced-resolve/blob/master/lib/CachedInputFileSystem.js#L238
There was a problem hiding this comment.
@DorianGrey Clarification on my question, it seems like the delegated callsite (https://github.com/webpack/enhanced-resolve/blob/master/lib/CachedInputFileSystem.js#L78-L80) omitted the utf8 encoding option, which we are doing here. I'm wondering what the implication of omitting the encoding option.
There was a problem hiding this comment.
A few data points:
-
Two-parameters seems to match how
webpackcalls that function, as per https://github.com/webpack/webpack/blob/eca7bad8de54c39b9cb8b138793362b8a17ac11b/lib/Compiler.js#L349 -
memory-fshas support for both two-parameter and three-parameter modes: https://github.com/webpack/memory-fs/blob/7ae73ebf34ae2f95266e6c3fef1f8e99f9d2308a/lib/MemoryFileSystem.js#L313
The one usage of readFileWrapper is to read in the swSrc, and the results of that are then used as part of a ES2015 template string to construct the final swDest file.
If the native fs.readFile() is called in two-parameter mode, without specifying the encoding, then it returns the contents as a Buffer rather than a string. But, using the Buffer within the final ES2015 template string will automatically call toString() on in, and the default encoding for that stringification is utf-8. So, we should have the same end result for the fs.readFile() case as well.
There was a problem hiding this comment.
👍 Awesome, thank you for the detailed explanation and references to the source code.
|
I'm not as familiar with this issue, but this change looks fine to me. @jeffposnick if you're fine with merging, +1 from me. |
| @@ -28,7 +28,7 @@ | |||
| */ | |||
| function readFileWrapper(readFileFn, filePath) { | |||
| return new Promise((resolve, reject) => { | |||
| readFileFn(filePath, 'utf8', (error, data) => { | |||
There was a problem hiding this comment.
A few data points:
-
Two-parameters seems to match how
webpackcalls that function, as per https://github.com/webpack/webpack/blob/eca7bad8de54c39b9cb8b138793362b8a17ac11b/lib/Compiler.js#L349 -
memory-fshas support for both two-parameter and three-parameter modes: https://github.com/webpack/memory-fs/blob/7ae73ebf34ae2f95266e6c3fef1f8e99f9d2308a/lib/MemoryFileSystem.js#L313
The one usage of readFileWrapper is to read in the swSrc, and the results of that are then used as part of a ES2015 template string to construct the final swDest file.
If the native fs.readFile() is called in two-parameter mode, without specifying the encoding, then it returns the contents as a Buffer rather than a string. But, using the Buffer within the final ES2015 template string will automatically call toString() on in, and the default encoding for that stringification is utf-8. So, we should have the same end result for the fs.readFile() case as well.
R: @jeffposnick @philipwalton
Fixes #1393.
Description of what's changed/fixed.
The issue linked above provides a more detailed explanation about the source of this problem.
Short version: The
InjectManifestversion ofworkbox-webpack-plugincurrently relies on an internal function_readFileof the filesystem API. This only works under specific circumstances, and is unreliable in a more general case (e.g. in case of a decorated filesystem version).Changes included in this PR:
readFilefunction ofwebpack's filesystem API. Requires an explicit binding forthis.readFileFnofreadFileWrapper- it cannot be provided explicitly in this case.