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

Paralellize image uploads #5535

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Paralellize image uploads #5535

merged 7 commits into from
Oct 1, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 30, 2024

I'm starting to bring in different pieces inspired by / taken from #4163. The first of these pieces will be moving decoupling link meta / embed resolution from composer previews. I'm not ready to do that yet so I'm starting by doing little refactorings.

In this refactor, I'm doing a few things to the post creation method:

  • Removed unsafe as coercions. They were only needed due to how the logic was structured with embed being redefined to wrap the previous embed.
  • I found the recursive definition of embed a bit difficult to trace in general because it wasn't clear which cases are mutually exclusive. I've refactored it with stronger types to actually follow the codepaths that can execute.
  • While doing so, I've added parallelization for image uploads — similar to how we already do for video.

Test Plan

Verify all of these still work.

  • Posting a link (with preview)
  • Posting one image
  • Posting multiple images
  • Posting a GIF
  • Posting a video
  • Posting a starter pack
  • Posting a list
  • Posting a feed
  • Posting a quote
  • Posting a quote with image(s)
  • Posting a quote with video
  • Posting a quote with GIF
  • Posting a quote with link

Copy link

github-actions bot commented Sep 30, 2024

Old size New size Diff
7.9 MB 7.9 MB -12 B (-0.00%)

Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

So much better! Pinged in Slack for another issue, but if we wanna punt on that, we should just discuss my one comment here.

src/lib/api/index.ts Outdated Show resolved Hide resolved
@arcalinea arcalinea temporarily deployed to embed-1 - social-app PR #5535 October 1, 2024 07:22 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to embed-1 - social-app PR #5535 October 1, 2024 07:25 — with Render Destroyed
Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

A beaut!

@gaearon gaearon merged commit a7ee561 into main Oct 1, 2024
6 checks passed
@gaearon gaearon deleted the embed-1 branch October 1, 2024 15:03
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.

3 participants