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

Makes brotli-size an optional dep #299

Closed
wants to merge 1 commit into from

Conversation

flovilmart
Copy link

Description

This PR removes Brotli-size as a dependency and puts it in the optional deps in order to provide node 10+ compatibility

Motivation and Context

The current implementation cannot be setup on any modern node system as Brotli-size fails to install due to native bindings not being available.

Changes

This is not a breaking change as users who require Brotli for size computation will see no change. Moving to a non supported version of node, Brotli-size will fail so install, which will in turn fail the test (which is better than failing to install this module altogether)

@flovilmart
Copy link
Author

@siddharthkp is it something you are considering in order to bring compatibility to node 10+ or do you wish to not integrate this change?

@siddharthkp
Copy link
Owner

siddharthkp commented Jul 2, 2019

@flovilmart, can you mention the exact versions when you say:

The current implementation cannot be setup on any modern node system as Brotli-size fails to install due to native bindings not being available.

Context: I am able to install brotli-size on Node 8, 10 and 12 🤔

@flovilmart
Copy link
Author

from what I recall, brotli-size depends on iltotb which provides native bindings for brotli. This project only publishes a handful of pre-built binaries. In certain environment, like a lightweight Docker builder image, the build tools necessary to build iltorb may not be installed. iltorb would then in turn not build, and this project would not install.

I believe the node version used was 10.15.3 on circle CI and heroku. It looks like the binaries are uploaded on iltorb releases now but that does not mean they were

I am not involved in the project that required this and perhaps @nicoespeon can track down the commit that made use of the fork in the project and provide additional context for this.

@siddharthkp siddharthkp mentioned this pull request Jul 3, 2019
@flovilmart flovilmart closed this Jan 19, 2021
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.

2 participants