Skip to content

Conversation

@Igloczek
Copy link
Contributor

What does it do?

Prevent returning optimized image if it's larger than the original. It's mostly visible on PNG images, and it's a known behavior of sharp lovell/sharp#1110, lovell/sharp#1017, lovell/sharp#600, lovell/sharp#478.

Why is it needed?

When the user is uploading an already optimized image, or just the optimizer is unable to reduce the size, we should keep the image that is smaller.

Related issue(s)/PR(s)

I didn't find any of them.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Makes sense indeed 👍

const optimized = await sharpInstance
.toBuffer({ resolveWithObject: true })
.then(({ data, info }) => ({
buffer: data,
Copy link
Member

Choose a reason for hiding this comment

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

can't you just do buffer.length < data.length here ? that would help us keep the simple return of promise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to edit the size param below too, but indeed it looks better that way.

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #9214 (75c1a40) into master (5d8f5fb) will decrease coverage by 0.00%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9214      +/-   ##
==========================================
- Coverage   34.64%   34.64%   -0.01%     
==========================================
  Files        1308     1308              
  Lines       14431    14446      +15     
  Branches     1432     1435       +3     
==========================================
+ Hits         5000     5005       +5     
- Misses       8517     8527      +10     
  Partials      914      914              
Flag Coverage Δ
front 26.03% <7.14%> (-0.02%) ⬇️
unit 54.92% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t-manager/admin/src/components/SelectMany/index.js 0.00% <ø> (ø)
...nt-manager/admin/src/components/SelectOne/index.js 0.00% <ø> (ø)
...anager/admin/src/components/SelectWrapper/index.js 0.00% <0.00%> (ø)
...in-content-manager/admin/src/translations/index.js 0.00% <0.00%> (ø)
...ntent-type-builder/admin/src/translations/index.js 0.00% <0.00%> (ø)
...trapi-plugin-email/admin/src/translations/index.js 0.00% <0.00%> (ø)
...rapi-plugin-upload/admin/src/translations/index.js 0.00% <0.00%> (ø)
...-users-permissions/admin/src/translations/index.js 0.00% <0.00%> (ø)
...kages/strapi-admin/admin/src/translations/index.js 100.00% <100.00%> (ø)
...pi-plugin-content-manager/controllers/relations.js 90.62% <100.00%> (+1.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d19888...6ed3c35. Read the comment docs.

@Igloczek
Copy link
Contributor Author

BTW. Is normal that running format script, edit like 150 files in the repository?

@alexandrebodin
Copy link
Member

alexandrebodin commented Jan 25, 2021

BTW. Is normal that running format script, edit like 150 files in the repository?

Yep the legacy code haven't been prettified to avoid conflicts so we only run them on commit (so on changed files only) :)
I think we will run it once before a major release that will also have breaking changes so it is less painfull

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM 💯 nice job

@alexandrebodin alexandrebodin added this to the 3.4.5 milestone Jan 25, 2021
@alexandrebodin alexandrebodin added source: core:upload Source is core/upload package issue: enhancement Issue suggesting an enhancement to an existing feature labels Jan 25, 2021
@alexandrebodin alexandrebodin merged commit 3f1fa39 into strapi:master Jan 25, 2021
@Igloczek Igloczek deleted the feature/image-optimalization branch January 25, 2021 17:21
@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/new-release-strapi-v3-4-5/2446/1

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

Labels

issue: enhancement Issue suggesting an enhancement to an existing feature source: core:upload Source is core/upload package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants