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

Make brotli-size optional by styfle #220

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

siddharthkp
Copy link
Owner

Description

  1. Remove brotli-size from dependencies
  2. Add try-catch around require('brotli-size')
  3. Warn if using brotli option but missing brotli-size

Motivation 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

Copy link

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Great stuff!

@styfle
Copy link

styfle commented May 2, 2018

Eventually, we might even get brotli support in Node Core.
See nodejs/node#20458

@XhmikosR
Copy link
Contributor

XhmikosR commented May 2, 2018

@siddharthkp: I currently have downgraded bundlesize to 0.15. This should be squashed and merged ASAP.

Node.js 10.0.0 is out for quite some time.

@siddharthkp
Copy link
Owner Author

siddharthkp commented May 3, 2018

@styfle Do you think the build should error out instead of passing if brotli-size is not installed?

(Check out the result of the build)

I have added you as a collaborator so you can commit directly to this branch (instead of a fork)

@styfle
Copy link

styfle commented May 3, 2018

@siddharthkp That's a good point since they specifically said they wanted to use brotli. Maybe the correct thing to do is fail fast and just throw instead of a warning:

throw new Error('Missing optional dependency. Install it with: npm install --save brotli-size')

Thoughts?

@eseliger
Copy link

eseliger commented May 3, 2018

I agree with you guys
Requesting to check the size based on brotli compression and getting an OK return code seems wrong

@styfle
Copy link

styfle commented May 3, 2018

@siddharthkp Ok the build is failing as expected now that there is a throw.

How should we fix the tests so that brotli-size is installed? Add to devDependencies?

@eseliger
Copy link

eseliger commented May 3, 2018

how about running it once and checking for failure and then running npm i brotli-size and running again and check for success?

@styfle
Copy link

styfle commented May 3, 2018

@eseliger That was my first thought but I don't see a test harness so I'm not sure how to assert that and error was thrown with the current test commands.

@eseliger
Copy link

eseliger commented May 3, 2018

hmm maybe some script like this will work

bundlezise

if [ $? -eq 0 ]
then
  echo "Should have failed"
  exit 1
else
  exit 0
fi

try {
brotli = require('brotli-size')
} catch (e) {
throw new Error(`Missing optional dependency. Install it with:
Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think you should throw an error here. I would say, the travis build should pass because it only invokes bundlesize and the bundlesize build status should error out with the right message.

This should help: https://github.com/siddharthkp/bundlesize/blob/master/src/build.js#L32

Copy link

Choose a reason for hiding this comment

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

I'm not sure I understand. Do you want to make the change on this branch?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yep, this branch

Copy link
Contributor

@jakebolam jakebolam May 6, 2018

Choose a reason for hiding this comment

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

I think siddharthkp was saying that you can make bundlesize exit with non-zero, so travis doesn't fail, but report bundlesize failure status to GitHub

In an offshoot version of bundlesize, we've elected to fail the build
https://github.com/bundlewatch/bundlewatch/blob/master/src/app/getLocalFileDetails/getSize.js#L12

Copy link
Owner Author

@siddharthkp siddharthkp May 7, 2018

Choose a reason for hiding this comment

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

I'd suggest the way to fail the bundlesize build would be the companion error method: https://github.com/siddharthkp/bundlesize/blob/master/src/build.js#L32

Copy link

Choose a reason for hiding this comment

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

@siddharthkp Would you like to implement this?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't mind, it might take me some time to get to this though 😅

@@ -1,5 +1,6 @@
const { warn } = require('prettycli')
Copy link
Collaborator

@palashmon palashmon May 5, 2018

Choose a reason for hiding this comment

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

There is a CI build error coming due to this line:

> eslint src store/*.js

/home/travis/build/siddharthkp/bundlesize/src/compressed-size.js
  1:9  error  'warn' is assigned a value but never used  no-unused-vars
✖ 1 problem (1 error, 0 warnings)

Please remove this line.

@XhmikosR
Copy link
Contributor

XhmikosR commented Jul 4, 2018

Hey, guys. Any news? I'm still using a downgraded version due to no node.js 10.x support.

@styfle
Copy link

styfle commented Jul 5, 2018

I think @siddharthkp still wanted to make a change here.

Another related issue worth pursuing is to get brotli support in Node Core.
Put a 👍 on this issue: nodejs/node#18964

@XhmikosR
Copy link
Contributor

@siddharthkp: friendly ping!

@roopakv roopakv mentioned this pull request Nov 1, 2018
4 tasks
@XhmikosR
Copy link
Contributor

Is the package abandoned @siddharthkp ?

@styfle
Copy link

styfle commented Jan 6, 2019

FYI, brotli support landed in Node core and should be in the next release.

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.

6 participants