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

Use native brotli when available and make iltorb & node-zopfli-es optional #36

Merged
merged 5 commits into from Apr 29, 2019

Conversation

@zetlen
Copy link

commented Apr 27, 2019

First off, thank you for maintaining this fork. Really appreciated.

Purpose

Make shrink-ray more fault-tolerant, so it can install and run without fatal errors on more versions of Node and build environments.

Ingredients

  • Change hard dependencies on binary packages iltorb and node-zopfli-es to peer dependencies, so consuming apps can control their exact versions.
  • Use native Brotli compression in NodeJS > 11.8 when available, and iltorb when not. When neither are available, fall back to gzip.
  • Use node-zopfli-es when available, and fall back to gzip when not.
  • Log warnings when fallbacks are in use.
  • Update node-zopfli-es and mocha to comply with security audits.
  • Add tests and proxyquire utility to simulate when modules are unavailable.

Compatibility

Tested locally on Node:

  • v8.10.0
  • v8.12.0
  • v10.14.2
  • v11.14.0
@coveralls

This comment has been minimized.

Copy link

commented Apr 27, 2019

Coverage Status

Coverage increased (+0.08%) to 99.155% when pulling e4e9122 on magento-research:magento/make-binary-deps-optional into 3018a26 on Alorel:master.

brotli-compat.js Outdated Show resolved Hide resolved
zopfli-compat.js Outdated Show resolved Hide resolved
@Alorel

This comment has been minimized.

Copy link
Owner

commented Apr 27, 2019

Hey, thanks for the PR. Looks good overall, just a couple of minor things. The Readme should also be updated with updated install instructions as the package now has peer dependencies.

CI is failing because Node 12 was just released and the native bindings haven't been updated for the new API version - that's fine and temporary.

zetlen added 2 commits Apr 28, 2019
…-deps-optional
@zetlen

This comment has been minimized.

Copy link
Author

commented Apr 28, 2019

Thanks for your prompt response, @Alorel ! I've switched to process.emitWarning and added README details.

zetlen added 2 commits Apr 28, 2019
@Alorel
Alorel approved these changes Apr 29, 2019
@Alorel

This comment has been minimized.

Copy link
Owner

commented Apr 29, 2019

Great, thanks 🙂 I can't make edits to a forked project, so as soon as you resolve conflicts with master it'll be good to merge (it should only be a node-zopfli-es version change in package.json)

@zetlen

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

@Alorel I resolved the package.json conflicts last night and GitHub say there are no conflicts. Am I missing something?

@Alorel

This comment has been minimized.

Copy link
Owner

commented Apr 29, 2019

Hm, it's showing up as conflicting on github.com, but mergeable on the FastHub app

@Alorel Alorel merged commit 40a3717 into Alorel:master Apr 29, 2019
4 checks passed
4 checks passed
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 99.155%
Details
security/snyk - package.json (Alorel) No new issues
Details
@Alorel

This comment has been minimized.

Copy link
Owner

commented Apr 29, 2019

Published as v4.0.0 - thanks again for the contribution!

@zetlen zetlen deleted the magento-research:magento/make-binary-deps-optional branch Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.