Skip to content

Conversation

@tad3j
Copy link

@tad3j tad3j commented Nov 4, 2025

Fixes issue #2545

Copy link
Member

@HassanBahati HassanBahati left a comment

Choose a reason for hiding this comment

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

Hi @tad3j, thank you for working on this Pull Request 👏

Can you please make the changed below;

  1. Please update CHANGELOG.md to reflect what has changed.
  2. Please bump the extension version in extension.yaml
  3. Can you please move the event trigger to generateResizedImage function

Cheers!

@github-project-automation github-project-automation bot moved this to Changes Requested [PR] in [Cloud] Extensions + Functions Nov 4, 2025
@tad3j tad3j force-pushed the fix/add-storage-resize-images-onstart-hook branch from e02bd62 to abb9f75 Compare November 5, 2025 09:09
@tad3j
Copy link
Author

tad3j commented Nov 5, 2025

Hi @tad3j, thank you for working on this Pull Request 👏

Can you please make the changed below;

  1. Please update CHANGELOG.md to reflect what has changed.
  2. Please bump the extension version in extension.yaml
  3. Can you please move the event trigger to generateResizedImage function

Cheers!

Hey @HassanBahati .

Updated md and yml files as requested and moved event call to parent function. Notice that I also changed the argument (only passing object without sending name twice) and added await.

@tad3j
Copy link
Author

tad3j commented Nov 5, 2025

Btw, with change above onStart event fires even if shouldResize returns false. Shouldn't we skip onStart event in this case? (that's why I initially added event triggering inside generateResizedImageHandler , after shouldResize checck)

@cabljac
Copy link
Contributor

cabljac commented Nov 5, 2025

Hey @tad3j that's a good point. The description of this particular event in the extension.yaml implies it is fired every time the function executes, but this is probably less useful here.

We will double check with the Firebase Extensions team to get their opinion on this.

I can think of three options:

  1. We filter firing the events by shouldResize (like in the original PR)
  2. We don't filter the events by shouldResize (but we still need the event adding, as in the current PR)
  3. We don't filter this event but we add a new event, something like "onStartResize"

@tad3j
Copy link
Author

tad3j commented Nov 5, 2025

Hey @tad3j that's a good point. The description of this particular event in the extension.yaml implies it is fired every time the function executes, but this is probably less useful here.

We will double check with the Firebase Extensions team to get their opinion on this.

I can think of three options:

  1. We filter firing the events by shouldResize (like in the original PR)
  2. We don't filter the events by shouldResize (but we still need the event adding, as in the current PR)
  3. We don't filter this event but we add a new event, something like "onStartResize"

My use-case is cleanup before resize:
I want to delete users profile images (resized) before resizing uploaded one (on staging env I saw error that images weren't resized if they already exist hence why I started looking into this event). If event is fired before shouldResize this means I would delete profile images but resize wouldn't even be attempted...leaving user without profile pic (I know exception can occur and the same would happen but this is also the case if images don't exist yet).

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