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

Restore to working order #2

Merged
merged 19 commits into from
Sep 3, 2019
Merged

Restore to working order #2

merged 19 commits into from
Sep 3, 2019

Conversation

alexjeffburke
Copy link
Collaborator

@alexjeffburke alexjeffburke commented Sep 3, 2019

This is somewhat of a large PR but it basically upgrades all the modules and puts in some glue to make it compatible with the latest version of sharp.

Oh, just to be transparent it obviously removes the babel build, but this isn't needed at all now that versions of node are out support async-await. That means as of node 8 all that was needed is swapping a coupe of places back to module.exports and the whole lot goes away making it far easier to build.

name = 'resize';
args = [ null, null, { fit: 'cover', position: args[0] } ];

const locatedOperation = locatePreviousCommand(operationsForExecution, name);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since crop and resize are now one and the same, this locatePreviousCommand stuff tries to compact the the commands back together - this can probably be removed for the purposes of removing any chance at functional change.

Copy link
Owner

Choose a reason for hiding this comment

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

Unless it's important for you to maintain backwards compatibility with express-processimage I think we can also just align with the new sharp api? At least if we find it more elegant :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh right yeah - need to remember that this isn’t necessarily meant to abstract the specific engine away but rather select the appropriate engine..

I’ve noticed this elsewhere but there’s obviously a tension between generic commands meaning the right engine is picked for the job versus specific commands that might expose more but tie the choices. My feeling is a handful of generic commands might be useful - particularly setFormat which became a big win in express-processimage but I’ve not thought it through fully.

let _ = require('lodash');
let mime = require('mime');
let Pipeline = require('./Pipeline');
var _ = require('lodash');
Copy link
Collaborator Author

@alexjeffburke alexjeffburke Sep 3, 2019

Choose a reason for hiding this comment

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

Err, I can return there to being let - that said lebab passes are probably desirable anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, lebab ftw. eslint --fix can also take care of many of those, if a good rule set is enabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah! My thought was if we land this as-is its be natural to then follow up separately with a prettier conversion and then run the latest eslint over the lot.

Copy link
Owner

Choose a reason for hiding this comment

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

Great! Yeah, let's do that.

Copy link
Owner

@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.

Thanks a lot for doing this!

@alexjeffburke alexjeffburke merged commit e2ef39e into papandreou:master Sep 3, 2019
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.

2 participants