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.

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

feat(server): Add support for encoded source files #3123

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

GreenGremlin
Copy link
Contributor

@GreenGremlin GreenGremlin commented Aug 31, 2018

This change allows preprocessors to specify an encoding for files. Currently, if a preprocessor encodes a file in a format such as gzip or Brotli, the file is served with the wrong encoding. The only workaround for this is to also include a plugin (example) that removes encoded files from files.served and serves them manually using a custom file handler. This approach is a gross hack that unnecessarily complicates the encoding plugin's logic.

With this change, a preprocessor encodes the contents and stores them in the File class' new encodings property. The encodings property is a keyed object where the keys are the encoding type ('gzip', 'compress', 'br', etc.) and the values are the encoded content. This pattern has the advantage of allowing multiple encoders and serving the encoding that best matches the request. It also allows the unencoded content to be served, when no encoding matches the request.

I know there has been pushback on adding encoding support, but I'm hoping this approach is an acceptable compromise. This solution leaves pretty much all encoding logic to the preprocessors and minimizes karma's role as much as possible.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@GreenGremlin
Copy link
Contributor Author

CLA signed

@googlebot
Copy link

CLAs look good, thanks!

@GreenGremlin GreenGremlin force-pushed the source-file-encoding branch 2 times, most recently from 1ac7de8 to 720d718 Compare August 31, 2018 19:26
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

You mentioned pushback, can you point to it so I can understand the issue. Your approach looks good to me.

Can you add a short bit of documentation to dev/plugins.html under preprocessors?
I'm thinking of a slight re-write of your commit description:

a preprocessor encodes the contents and stores them in the File class' new encodings property. The encodings property is a keyed object where the keys are the encoding type ('gzip', 'compress', 'br', etc.) and the values are the encoded content.

lib/file.js Show resolved Hide resolved
test/unit/middleware/source_files.spec.js Outdated Show resolved Hide resolved
@GreenGremlin
Copy link
Contributor Author

You mentioned pushback, can you point to it so I can understand the issue. Your approach looks good to me.

I updated the description and added a link to [the issue](This #1241) in which there was pushback.

@GreenGremlin
Copy link
Contributor Author

Can you add a short bit of documentation to dev/plugins.html under preprocessors?
I'm thinking of a slight re-write of your commit description:

Done. It felt odd to document only the encodings property of the file object, so I added documentation for all three arguments passed to preprocessors. For the file argument, I documented the path, originalPath, encodings, and type properties. I did not document contentPath, mtime, isUrl, and doNotCache; because they seemed more internal to karma and not really related to preprocessors. Let me know if you'd like me to document any of the properties I did not document, or if you'd like me to remove any of the properties I did document.

@GreenGremlin GreenGremlin force-pushed the source-file-encoding branch 2 times, most recently from 554b550 to 0e0d5ad Compare September 5, 2018 02:16
@johnjbarton
Copy link
Contributor

I think your feature is more general than the feature request that was rejected earlier. It fits into karma's architecture, allowing other plugins without a large impact on the core code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants