Make brotli-size optional #214
Conversation
|
@siddharthkp Does travis need to install optional dependencies to run tests? |
package.json
Outdated
| @@ -49,6 +48,9 @@ | |||
| "prettycli": "^1.4.3", | |||
| "read-pkg-up": "^3.0.0" | |||
| }, | |||
| "optionalDependencies": { | |||
| "brotli-size": "0.0.1" | |||
There was a problem hiding this comment.
Should this be updated to 0.0.2 at the same time? :-)
There was a problem hiding this comment.
Hey that sounds like a good idea considering the size of the package decreased significantly https://packagephobia.now.sh/[email protected]
There was a problem hiding this comment.
Are there any difference in size because of that?
styfle
changed the title
src/compressed-size.js
Outdated
| const brotli = require('brotli-size') | ||
| size = brotli.sync(data) | ||
| } catch (e) { | ||
| console.warn('Missing optional dependency: brotli-size') |
There was a problem hiding this comment.
can we make this slightly more instructional + use prettycli (already declared as dependency), that will let you add nice multiline warnings.
WARN Missing optional dependency. Install it with:
npm install brotli-size
|
@siddharthkp @edmorley Any other changes requested? |
package.json
Outdated
| @@ -49,6 +48,9 @@ | |||
| "prettycli": "^1.4.3", | |||
| "read-pkg-up": "^3.0.0" | |||
| }, | |||
| "optionalDependencies": { | |||
| "brotli-size": "0.0.2" | |||
There was a problem hiding this comment.
I'm pretty sure that both npm and yarn install optionalDependencies by default (they just don't exit with a non-zero exit code, if it wasn't able to be installed).
As such with default npm yarn options (which is what most users will use), this PR will solve the installation errors issue, however there will still be install time and size impact from brotli-size. Perhaps instead brotli-size should be removed from package.json entirely - and then users will install manually instead? (Which is what the error message added to src/compressed-size.js says to do anyway; though perhaps it would be good to also have a README.md mention?)
Many thanks for looking into this!
There was a problem hiding this comment.
@edmorley You're right! I changed to peerDependency since this will give a hint to a developer looking at package.json
I also added a note to the README 👍
There was a problem hiding this comment.
Unfortunately having in peerDependencies will result in warnings being shown when installing with yarn/npm, if it's not installed. Some users fail CI if these warnings are generated. I think the best plan is to omit it from package.json entirely.
There was a problem hiding this comment.
@edmorley Yes I can see how that would be misleading for the users who only want to use gzip. I removed it completely now, thanks! 👍
|
@siddharthkp @edmorley I think I got it right this time 😄 |
styfle
changed the title
styfle
changed the title
|
I updated the title and description to reflect the changes requested |
|
@siddharthkp: this is an issue even more with node.js 10.x and bundle-size depending on iltorb which doesn't have the bindings yet. |
src/compressed-size.js
Outdated
| const brotli = require('brotli-size') | ||
| size = brotli.sync(data) | ||
| } catch (e) { | ||
| warn(`Missing optional dependency. Install it with: |
There was a problem hiding this comment.
If something else can throw too in brotli.sync, then the message would be misleading, wouldn't it?
There was a problem hiding this comment.
@XhmikosR Good point. I fixed this 🔧
| @@ -50,6 +50,8 @@ npx bundlesize | |||
| #### 1) Add the path and maxSize in your `package.json`. | |||
| By default the gzipped size is tested. You can use the `compression` option to change this. (`gzip`, `brotli`, or `none`). | |||
|
|
|||
| To use the `brotli` compression option, you must install the peer dependency: `npm install --save brotli-size` | |||
There was a problem hiding this comment.
"the brotli-size peer dependency" maybe?
There was a problem hiding this comment.
I think this is pretty clear already since it has the exact command to run. This is how it looks rendered:
|
Ping @siddharthkp |
|
@XhmikosR Hey! Sorry I was on a laptop less vacation. Will test this out soon |
|
Merged into local branch so that CI tests can run. New PR: #220 |
|
@siddharthkp: thanks! I was going to downgrade bundle-size to be honest if it took more time because this fails for node 10.x. |
|
@siddharthkp Thanks, anything else I need to do? |
Description
brotli-sizefromdependenciesrequire('brotli-size')brotli-sizeMotivation and Context
It turns out the
brotli-size(#194) is quite big and doesn't install very nicely on Windows.See the following issues:
Fixes #202
Fixes #213