Extend node-hubble to allow discovery of image sizes

RESOLVED FIXED

Status

--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: humph, Assigned: humph)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
We would like to know when to use an image based on its size, when to crop it, etc.  Hubble is a perfect candidate for this.  I've written the code and tests, just need to fixup the readme, then I'll get this into review.

WIP here - https://github.com/mozilla/node-hubble/pull/13
(Assignee)

Comment 1

6 years ago
Created attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

Finished updating README, this is ready for review
Attachment #747407 - Flags: review?(schranz.m)
Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

The code looks very sane to me, but I'm getting test fails when running locally. Specifically:

1) should give proper sizes for JPEG files
2) should give proper sizes for GIF files

And assertion errors on status codes. This leaves me thinking I'm somehow missing a dependency for the tests? I'm unsure, but everything I needed installed successfully with npm.
Attachment #747407 - Flags: review?(schranz.m)
Attachment #747407 - Flags: review?(jon)
Attachment #747407 - Flags: review-
(Assignee)

Comment 3

6 years ago
Take a look at your node_modules/canvas/build/canvas.target.mk, and see if gyp found the right deps.  It should have defined HAVE_JPEG and HAVE_GIF.  Here's what I have after I build:

DEFS_Release := \
	'-D_DARWIN_USE_64_BIT_INODE=1' \
	'-D_LARGEFILE_SOURCE' \
	'-D_FILE_OFFSET_BITS=64' \
	'-DHAVE_FREETYPE' \
	'-DHAVE_JPEG' \
	'-DHAVE_GIF' \
	'-DBUILDING_NODE_EXTENSION'
Flags: needinfo?(schranz.m)
Yeah, I'm missing JPEG and GIF from that list. Rather than poke around more I'll let :jbuck look at it. Code R+ from me.
Flags: needinfo?(schranz.m)
(Assignee)

Comment 5

6 years ago
Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

Kind sir, a moment of your time?
Attachment #747407 - Flags: review?(jon) → review?(jon)

Comment 6

6 years ago
Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

r+, although I'm not a huge fan of the node-canvas dependency. It'd be nice to have something pure JS... if only we had unlimited time!
Attachment #747407 - Flags: review?(jon) → review+
(Assignee)

Comment 7

6 years ago
https://github.com/mozilla/node-hubble/commit/e30151186941e841721312747b16bf4d4977e6d6

The node-canvas dep isn't a big deal for a server.  Basically, once it's doable in JS, node-canvas will switch to it too, so we'll all be happy.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.