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

Already on GitHub? Sign in to your account

Fix serving binary files #885

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

fixes #864

Well that didn't work :/, but I am certain converting the buffer to a string for everything is breaking images.

Well, I'm really at a loss as an outside contributor on this one. This change breaks all the karma preprocessors out there that expect a string as the content argument. Which includes all maintained by the karma-runner organization.

The correct way to fix this IMO would be to pass a buffer to all preprocessors, and let them convert to a string as needed. Though the only way I can think about doing this with backwards compatibility is to check the mime type, and only convert to string if it is a text file.

For now, I leave this one up to the maintainer(s) for the solution, but at least this one is tracked down.

Contributor

vojtajina commented Jan 21, 2014

Sorry for delays @drkibitz, I'm looking into it...

Contributor

vojtajina commented Jan 21, 2014

I'm not sure what the best solution/fix is.

If I was strongly for using buffers, I would break the API. I'm however not sure buffers are the best in this case.
Does it make any sense to preprocess binary data? I can't think of any reasonable use case. Therefore I assume preprocessors are for plain text (say UTF8). Most of them (basically all) mutate the data. That means, each preprocessor has to convert the buffer to a string and then it returns a string and we need to make sure to create a buffer from it, before calling the next preprocessor (or make preprocessors to return buffers as well).

So at this point, I'm thinking about using buffers for binary content and probably disable preprocessing of binaries.

I see your point of view. Really the issue this PR is related to is about serving images correctly. If binary files skip preprocessors completely to fix this, that's totally fine. If at some point any unforeseen use case of binary preprocessors comes up, it can be a separate pipeline such as binaryProprocessors or something ;)

@vojtajina vojtajina added a commit that referenced this pull request Feb 5, 2014

@vojtajina vojtajina fix: serving binary files
For binary files, we can't cache the content as a string (utf8), we need to use a binary buffer.

All the preprocessors are currently implemented to deal with strings, as that makes sense for text files. If we changed preprocessors to deal with buffers then each preprocessor would have to convert the buffer to a string, do the string manipulation and then convert it back to a buffer. I don't think that's worthy as there are no binary preprocessors.

This change disables preprocessing of binary files to avoid weird errors.
It shows warning. If there is a reasonable use case for preprocessing binary files we can figure out something.

Closes #864
Closes #885
ca6bc8d

@vojtajina vojtajina added a commit that referenced this pull request Feb 5, 2014

@vojtajina vojtajina fix: serving binary files
For binary files, we can't cache the content as a string (utf8), we need to use a binary buffer.

All the preprocessors are currently implemented to deal with strings, as that makes sense for text files. If we changed preprocessors to deal with buffers then each preprocessor would have to convert the buffer to a string, do the string manipulation and then convert it back to a buffer. I don't think that's worthy as there are no binary preprocessors.

This change disables preprocessing of binary files to avoid weird errors.
It shows warning. If there is a reasonable use case for preprocessing binary files we can figure out something.

Closes #864
Closes #885
355994d

@vojtajina vojtajina closed this in 8a30cf5 Feb 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment