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

POC: itgalaxy/favicons#221 use sharp instead of phantomjs #254

Merged
merged 2 commits into from
Mar 11, 2019
Merged

POC: itgalaxy/favicons#221 use sharp instead of phantomjs #254

merged 2 commits into from
Mar 11, 2019

Conversation

kghost
Copy link
Contributor

@kghost kghost commented Dec 21, 2018

No description provided.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Awesome, sharp is good choice

@alexander-akait
Copy link
Member

/cc @brunocodutra

@kghost
Copy link
Contributor Author

kghost commented Dec 21, 2018

There are still issues about how sharp stretch images, I'll try fix it soon

@alexander-akait
Copy link
Member

@kghost also need fix CI

@kghost
Copy link
Contributor Author

kghost commented Dec 23, 2018

All commit tested.

  1. https://travis-ci.org/kghost/favicons/builds/471366944 Pass
  2. https://travis-ci.org/kghost/favicons/builds/471367724 Fail,
    Snapshots are generated by sharp. Resemble has problem comparing small pictures, favicon-16x16.png difference (misMatchPercentage) is 13%, above the threshold 5%. Also prove that image compare actually works.
  3. https://travis-ci.org/kghost/favicons/builds/471384328 Pass

.babelrc Outdated Show resolved Hide resolved
test/Image.js Show resolved Hide resolved
.gitignore Show resolved Hide resolved
src/helpers.js Outdated Show resolved Hide resolved
test/array.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@brunocodutra brunocodutra left a comment

Choose a reason for hiding this comment

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

Do you think we can use the new image comparator for all unit tests in place of AVA's snapshot?

src/helpers.js Show resolved Hide resolved
@kghost
Copy link
Contributor Author

kghost commented Jan 31, 2019

This pr already takes too long, I suggest merge it and improve it later.

@brunocodutra
Copy link
Collaborator

@kghost I raised a question above - is there any reason why the new image comparator was not used for all unit tests?

Either all tests should use AVA snapshots or they should all use the image comparator, but having both methods coexist would make it harder to maintain this library.

@pniederlag
Copy link

Since a recent update on my devbox(debian) phantomjs seems completly broken.

I extracted the main patch to switch from svg2png to sharp and rebased it to master. https://github.com/pniederlag/favicons/tree/pr/254-part1

Good News: this way I can generate the favicons again.

It would be really cool to get this Fix shipped and packaged. Big thx for putting your efforts into this!

@alexander-akait
Copy link
Member

Can we rebase?

@kghost
Copy link
Contributor Author

kghost commented Feb 15, 2019

Coming back from spring vacation, I'll fix the remaining problem next week.

@alexander-akait
Copy link
Member

Also please rebase 👍

@kghost
Copy link
Contributor Author

kghost commented Mar 11, 2019

ping @brunocodutra

@RomanHotsiy
Copy link

@brunocodutra do you plan to cut a new release anytime soon?
Thanks!

@brunocodutra
Copy link
Collaborator

I was hoping to address #253 before releasing, but if there's demand for a release meanwhile I can cut one.

@RomanHotsiy
Copy link

Ah, great. No need to hurry. Thanks for the update!

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.

5 participants