Fix file info size check #476
Conversation
core.js
Outdated
| @@ -74,7 +74,7 @@ async function _fromTokenizer(tokenizer) { | |||
| const checkString = (header, options) => check(stringToBytes(header), options); | |||
|
|
|||
| // Keep reading until EOF if the file size is unknown. | |||
| if (tokenizer.fileInfo.size === 0) { | |||
| if (tokenizer.fileInfo.size === undefined || tokenizer.fileInfo.size === null || tokenizer.fileInfo.size === 0) { | |||
There was a problem hiding this comment.
It's specifically undefined and not null, but I figured it would be better to accept both?
It's also never 0 (in the tests at least), I think before they are using undefined to indicate that the size hasn't been set yet. So potentially we want to remove 0? 馃
There was a problem hiding this comment.
@Borewit The .size property is not documented to be undefined. The docs should use number? instead of number to make that clear.
There was a problem hiding this comment.
@LinusU Let's only check for undefined.
There was a problem hiding this comment.
I just started looking into the failing tests as well. I noticed 2 things:
- The
fileInfo.sizeis, for some reason, not set reading from a file-stream (fs.createReadStream) includes the original file path which is used to determine the file size)
2. There are casesThe filefileInfo.sizeisundefined(because the stream length is not known), so that code needs to be improved.fileInfo.sizeis overwritten toNumber.MAX_SAFE_INTEGERin case the length is unknown.
I don't directly remember where the 0 is coming from. I will try to give some clarity on that and improve the documentation accordingly.
There was a problem hiding this comment.
Issue with size being 0 introduced here: https://github.com/sindresorhus/file-type/blame/331b1620c0d6a29b70a2e8e6c246cb2bdd93b183/core.js#L73
There was a problem hiding this comment.
Issue with size being 0 introduced here: https://github.com/sindresorhus/file-type/blame/331b1620c0d6a29b70a2e8e6c246cb2bdd93b183/core.js#L73
Yes, the linter auto-fix assumed .size would be a number and never undefined. It doesn't read the types. The previous check was too loose anyway though, so it's good that we make it more strict now.
|
Thank for looking into this 馃檶 |
|
I am puzzled, so we went from: if (!tokenizer.fileInfo.size) // works, but linter does not like itto if (!tokenizer.fileInfo.size === 0) // does not workto if (tokenizer.fileInfo.size === undefined || tokenizer.fileInfo.size === null || tokenizer.fileInfo.size === 0) // could be simplified to...Are we working for the linter or the linter for us? |
|
@Borewit I actually think that the last one of those is the better since when reading the code it's obvious that the value could be |
Fixing the failing tests as requested here #472 (comment) 馃槑