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.

Sign up for GitHub

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

Jump to bottom

Reduce external dependencies #633

Merged
merged 19 commits into from
Mar 7, 2019
Merged

Reduce external dependencies #633

merged 19 commits into from
Mar 7, 2019

Conversation

Munter
Copy link
Member

@Munter Munter commented Oct 18, 2018 •
edited by papandreou
Loading

Refs #627
Fixes #625

  • Remove jpegtran, pngquant, optipng, pngcrush and gifsicle OS-level dependency and install instructions. Already part of npm install
  • Update to newer histogram implementation without an OS-level dependency on cairo, libjpeg, libgif and libpango
  • Remove all unnessessary OS-level dependencies mentioned in README, travis config and Dockerfile

We should also consider moving node-zopfli optionalDependency into devDependency and just not spam the users with failed installation messages for optional depednencies, which npm really puts to much emphasis on. The fallback at runtime is that assetgraph emits an event about a possible improvement if you install node-zopfli, but there will be no failure.

@papandreou
Copy link
Member

This has be accompanied by express-processimage's dependency list also being slimmed down, right? https://github.com/papandreou/express-processimage/blob/195e3e86c41beb0dfc019cae71c492ac1301dfe8/package.json#L11-L29

@Munter
Copy link
Member Author

Munter commented Oct 19, 2018

The point of removing these OS-level dependencies are that the node-installed versions of these already come with their binaries. So they're essentially just noise in our installation instructions. The npm dependencies are what makes it continue to work

@Munter
Copy link
Member Author

Munter commented Oct 19, 2018

The true test is of course removing those in the travis config. Coming up...

@papandreou
Copy link
Member

Ah, okay, makes sense 👍

Munter added a commit to assetgraph/assetgraph-sprite that referenced this pull request Oct 20, 2018
@Munter
Replace optional canvas depedency with non-optional canvas-prebuilt d…
a6a0de5 
…ependency. Refs assetgraph/assetgraph-builder#633
@Munter
Copy link
Member Author

Munter commented Oct 20, 2018

This last build error was intentional. I've removed the dependencies needed to build assetgraph-sprite in the current version. I expect the build to go green when we depend on the next version with cairo external dependency removed

@Munter Munter changed the title Remove external dependencies on jpegtran, optipng, pngcrush and pngqu… Reduce external depednencies Oct 20, 2018
@Munter Munter requested a review from papandreou October 20, 2018 15:33
papandreou
papandreou reviewed Oct 20, 2018
View reviewed changes
Copy link
Member

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

Looks great -- but let's get the new ag-s released so we can see it go green?

@papandreou
Copy link
Member

We should also consider moving node-zopfli optionalDependency into devDependency and just not spam the users with failed installation messages for optional depednencies, which npm really puts to much emphasis on. The fallback at runtime is that assetgraph emits an event about a possible improvement if you install node-zopfli, but there will be no failure.

I agree. Since there are no tests of the gzip transform, we can just remove the dependency altogether. I'll look into that.

@papandreou papandreou mentioned this pull request Oct 20, 2018
Merged
@Munter Munter force-pushed the slim-dependencies branch from 082d7be to 779265b Compare October 21, 2018 09:36
@Munter Munter changed the title Reduce external depednencies Reduce external dependencies Oct 21, 2018
@Munter Munter force-pushed the slim-dependencies branch 2 times, most recently from 32c1cd4 to 1306a81 Compare December 1, 2018 13:18
@Munter
Copy link
Member Author

Munter commented Dec 1, 2018

Looks like there is a problem with express-processimage somewhere between 8.0.0 and 8.0.2 which causes Travis and my local machine to both fail with: https://travis-ci.org/assetgraph/assetgraph-builder/jobs/462165078#L748-L759

@papandreou
Copy link
Member

I think I've seen that failure on CI before, but I can't reproduce it locally :(

This was referenced Dec 31, 2018
Closed
Closed
Munter added 10 commits March 4, 2019 23:27
@Munter
Remove external dependencies on jpegtran, optipng, pngcrush and pngqu…
27b3821 
…ant. Depend on binaries through npm dependencies. Refs #627
@Munter
Remove unused global package dependencies
9682f89
@Munter
Update historgram to a version that doesn't depend on cairo
54ce841
@Munter
Move historgram and assetgraph-sprite to direct depednencies and remo…
e5e5ee3 
…ve eliminated external dependencies
@Munter
Update to assetgraph-sprite 3.0.1
970ac93
@Munter
Move assetgraph-sprite and historgram to direct dependencies
94201c4
@Munter
Updated OSX external dependency installation instructions
e2e4d44
@Munter
Update express-processimage to 8.0.2 to avoid 'os.tmpDir' deprecation…
2bb6413 
… warnings
@Munter
Replace bluebird with native promises and util.promsify. Use p-map wh…
d15158f 
…ere concurrency control was needed
@Munter
Revert "Update express-processimage to 8.0.2 to avoid 'os.tmpDir' dep…
f53546e 
…recation warnings"

This reverts commit 1a612e1.
@Munter Munter force-pushed the slim-dependencies branch from 67b9470 to b6aa34a Compare March 5, 2019 00:21
@papandreou
Copy link
Member

I fixed the express-processimage problem on master and upgraded to 8.1.0.

@papandreou
Merge branch 'master' into slim-dependencies
0c080b2 
* master:
  Run optipng before pngcrush
  Update express-processimage to version 8.1.0
  CHANGELOG: Include regular commits
  npx offline-github-changelog > CHANGELOG.md
@papandreou
Copy link
Member

@Munter, can we merge this now, or is there still work left to do?

@Munter
Copy link
Member Author

Munter commented Mar 7, 2019

I think the things I really wanted to fix are done. There's probably possible optimizations left, but at least the reduction to only two optional OS-level installations is nice. So I think we should merge

papandreou
papandreou approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

Excellent!

@Munter Munter merged commit 4cb79dc into master Mar 7, 2019
@Munter Munter deleted the slim-dependencies branch March 7, 2019 14:43
@plroebuck
Copy link

Sweet! What release?

@papandreou
Copy link
Member

papandreou commented Mar 7, 2019 •
edited
Loading

@plroebuck, 6.10.0, released 4 seconds ago 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@papandreou papandreou papandreou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Munter @papandreou @plroebuck