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

Reduce package size #3319

Merged
merged 2 commits into from Aug 6, 2018
Merged

Reduce package size #3319

merged 2 commits into from Aug 6, 2018

Conversation

jugglinmike
Copy link
Member

We've received a couple reports (gh-3316 and gh-3318) that the installation size of JSHint increased drastically in the recently-released version 2.9.6. This is easily verified using npm's --production flag:

mkdir nodemods
git checkout 2.9.5
npm install --production
mv node_modules nodemods/2.9.5
git checkout -- package.json
git checkout 2.9.6
npm install --production
mv node_modules nodemods/2.9.6
du -h -d 0 nodemods/*

Output:

9.9M    nodemods/2.9.5
277M    nodemods/2.9.6

Two independent commits contributed to this 20-fold increase in side. This patch addresses each of them in turn. You can verify using the same technique:

git checkout shrink-deps~
npm install --production
mv node_modules nodemods/2.9.6-phantom
git checkout -- package.json
git checkout shrink-deps
npm install --production
mv node_modules nodemods/2.9.6-phantom-unicode
du -h -d 0 nodemods/*

Output:

9.9M    nodemods/2.9.5
277M    nodemods/2.9.6
149M    nodemods/2.9.6-phantom
18M     nodemods/2.9.6-phantom-unicode

The final size is still twice that of the prior release, but that is due to the upgrade to Lodash 4 (gh-3260 and gh-3283).

9.9M    nodemods/2.9.6-phantom-unicode-lodash

Since that was motivated by security concerns, I'm more willing to accept the size increase. If this continues to be a concern for our consumers, we can consider instead relying on Lodash's sub-components.

Also of interest: the remove-dev-dependencies.js introduced here is not technically necessary on TravisCI because JSHint can now be successfully be installed in TravisCI on Node 0.8 and 0.10. That's surprising because this was all motivated by failures in TravisCI back in November. At first, I thought that maybe the maintainers of requests had changed their tune, but no such luck. TravisCI now pre-installs PhantomJS, so the offending ES2015 code (used to fetch the binary) is no longer evaluated on any GNU/Linux build. However, we're not so lucky in the AppVeyor-provided Windows environment (and even if we were, we probably shouldn't rely on implicit aspects of the build environments). That's why I've written a script to manually remove the offending packages prior to installation in CI.

I'd like to release a new version (i.e. 2.9.7) for this changeset so that we can help our consumers avoid the inefficiency as soon as possible.

The `phantom` and `phantomjs-prebuilt` modules cannot be installed in
legacy versions of Node.js because during their installation procedure,
they import code which uses ES2015 language features. When executed in
legacy versions of Node.js, this causes a syntax error, interrupting
installation and making it impossible to execute the project's tests.

A previous commit [1] addressed this issue by marking the dependencies
as "optional." This allowed the installation procedure to fail, and
because the packages are only necessary in the latest release of
Node.js, the relevant tests could execute as expected.

Unfortunately, this modification had an unintended effect on consumers
of JSHint: it forced them to also install the aforementioned packages.
Because these packages are only necessary when contributing to JSHint
(not when using it) and because they include large binary assets, this
had a non-trivial impact on the time and disk space required to install
JSHint.

Revert the previous change, marking the packages as "dev dependencies"
once again. Ensure that the project's tests can continue to execute in
legacy versions of Node.js by introducing a script that removes the
offending packages prior to installation on those platforms.

[1] 2f6ac13
The `unicode-5.2.0` npm module is not necessary when running JSHint; it
is included in this project only during development for the purposes of
generating static data. Relocate the reference to the package in the
project's manifest so that it is not needlessly downloaded by consumers
during typical package installation. Simplify the related logic which
dynamically loaded the latest unicode module to instead load via a
static string value.
@coveralls
Copy link

coveralls commented Aug 5, 2018

Coverage Status

Coverage remained the same at 98.755% when pulling 7806524 on jugglinmike:shrink-deps into d5c1a00 on jshint:master.

@rwaldron
Copy link
Member

rwaldron commented Aug 6, 2018

Wow, nice work to solve this!

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

Successfully merging this pull request may close these issues.

None yet

3 participants