Skip to content

Optimize images#1367

Merged
nikosdouvlis merged 5 commits intomainfrom
nikos/js-439-compress-user-uploaded-images
Jun 16, 2023
Merged

Optimize images#1367
nikosdouvlis merged 5 commits intomainfrom
nikos/js-439-compress-user-uploaded-images

Conversation

@nikosdouvlis
Copy link
Copy Markdown
Member

@nikosdouvlis nikosdouvlis commented Jun 16, 2023

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

This is the complimentary clerk-js work , for more information and examples go to: https://github.com/clerkinc/cloudflare-workers/pull/31

Introduce a makeResponsive HOC that adds the srcset attribute on all images served by the img-service worker.

Primitives shouldn't be aware of any Clerk-specific logic. All the image optimization logic is now extracted into its own makeResponsive HOC.

Apart from resizing te image, more optimizations are now enabled:

  • dynamic format negotiation (avif and webp support, falls back to jpeg when needed). This offers up to ~50% size reduction.
  • scale down to a max width of 1920
  • set a default quality of 80
  • respect height, width, fit and quality params

Before:
image

After:
image

Primitives shouldn't be aware of any Clerk-specific logic. All the image optimizatiion logic is now extracted into its own makeResponsive HOC.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 16, 2023

🦋 Changeset detected

Latest commit: 7fcea03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@clerk/clerk-js Patch
@clerk/shared Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/clerk-react Patch
@clerk/remix Patch
gatsby-plugin-clerk Patch
@clerk/nextjs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Copy Markdown

@jit-ci jit-ci Bot left a comment

Choose a reason for hiding this comment

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

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

@nikosdouvlis nikosdouvlis changed the title Nikos/js 439 compress user uploaded images Optimize images Jun 16, 2023
th: React.JSX.IntrinsicElements['th'];
tr: React.JSX.IntrinsicElements['tr'];
td: React.JSX.IntrinsicElements['td'];
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ Couldn't we use Pick<React.JSX.IntrinsicElements, 'div' | 'input' | ...> ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry @dimkl, not sure I get the question here :(
This is not used as a union but rather as a map internally, so changing this would probably mean a bigger refactor of the styled-system types.

srcUrl.searchParams.append('height', optimizedHeight.toString());
src = srcUrl.toString();
}
const src = imageUrl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙃 could we drop this line and use the imageUrl instead of src?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done!

optimize,
rounded = true,
imageFetchSize = 64,
imageFetchSize = 80,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ why 80?

initials?: string;
imageUrl?: string | null;
imageFetchSize?: number;
optimize?: boolean;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ isn't this a breaking change? Couldn't someone use our components (eg OrganizationAvatar) with the optimize prop?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is just an internal prop, not really needed right now as the new worker and HOC handles this automatically

'https://img.clerk.dev/',
'https://img.clerkstage.dev/',
'https://img.lclclerk.com/',
];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙃 i would keep it in shared (and also keep the isClerkImage and the below Framework agnostic utilities).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This makes sense to me, however, I think these are specific enough to have them colocated with the HOC

@nikosdouvlis nikosdouvlis merged commit 59bc649 into main Jun 16, 2023
@nikosdouvlis nikosdouvlis deleted the nikos/js-439-compress-user-uploaded-images branch June 16, 2023 09:38
@clerk-cookie clerk-cookie mentioned this pull request Jun 16, 2023
@clerk-cookie
Copy link
Copy Markdown
Collaborator

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@clerk clerk locked as resolved and limited conversation to collaborators Jun 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants