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

Batch progress completion incorrect when using mock sender #626

Closed
mwdiaz opened this issue Nov 20, 2023 · 9 comments
Closed

Batch progress completion incorrect when using mock sender #626

mwdiaz opened this issue Nov 20, 2023 · 9 comments
Assignees
Labels
bug Something isn't working completed

Comments

@mwdiaz
Copy link

mwdiaz commented Nov 20, 2023

Describe the bug
As of v1.5.0 (I believe due to the change made in #566), the completed property for a given Batch when using useBatchProgressListener reports the incorrect percentage if also using a mock sender.

I think this is due to using the underlying file's size here when determining the total size of the batch. When using a mock sender, the file size is determined by the fileSize option passed to getMockSenderEnhancer, but there is still a real underlying File object that will report the correct size and not the mocked size.

It might be nice if the progress timers for each mocked request did use the actual file size instead of a hard-coded value in the mock sender, since that might add a dash of realism to interactive examples in Storybook, but I understand if that isn't feasible for technical reasons. Regardless, having the batch progress work accurately in this scenario (as was the case prior to v1.5.0) is useful for us since where we display an overall progress bar along with individual item progress bars and use a mock sender in our design system (Storybook / Chromatic).

To Reproduce
Steps to reproduce the behavior:

  1. Create an Uploader with a mock sender enhancer (for example, following the example on the API docs page for mockSender)
  2. In a child component of <Uploader>, utilize the useBatchProgressListener hook and observe that the completed property for the batch is incorrect, given the progress intervals for the file being "sent"

Expected behavior
Batch progress should accurately reflect mocked items progress intervals when using mock-sender.

Versions
v1.5.0+

Code
Here's a simple CodeSandbox demonstrating the issue: https://codesandbox.io/s/muddy-dream-7jy765. If the @rpldy/* dependencies are reverted to 1.4.1. or earlier, the behavior will be as expected.

@yoavniran yoavniran self-assigned this Nov 21, 2023
@yoavniran yoavniran added the bug Something isn't working label Nov 21, 2023
@yoavniran
Copy link
Collaborator

hey @mwdiaz
Thanks for the detailed & complete bug report 👍
Will fix very soon

P.S. When I wrote the mock-sender I mostly thought about Uploady internal tests and faster dev time for the library. Its really cool to see that others use it as well and it makes a lot of sense, of course.

@mwdiaz
Copy link
Author

mwdiaz commented Nov 21, 2023

Thanks! Yeah, the mock sender has been great for us; both in interactive Storybook examples as well as unit tests.

Copy link
Contributor

github-actions bot commented Dec 2, 2023

It's been a while. Waiting for an update... 🧐

@yoavniran
Copy link
Collaborator

hey @mwdiaz, Uploady 1.7.0-rc.0 is available now with a fix for the issue 🤞.
mock-sender will now default to using the file size from the uploaded items, rather than the config value.

Note that if the config value is provided, it will be used so if you've added it to the initialization of the mock-sender, you should remove it.

Let me know if it works for you.

@yoavniran yoavniran removed the no-stale label Dec 3, 2023
@mwdiaz
Copy link
Author

mwdiaz commented Dec 5, 2023

Thanks! I'm out of the office on vacation, but will definitely check it out when I'm back next week.

@mwdiaz
Copy link
Author

mwdiaz commented Dec 11, 2023

Just tried it out and things look better! One thing I've noticed is that for batch progress events the completed value is not multiplied by 100 before being passed to the consumer, which is different from items. Is there a technical reason for that, or could the behavior be standardized? I can see it being a potential source of confusion.

Thanks for the quick fix, when do you think a non-RC version might be made available on npm?

@yoavniran
Copy link
Collaborator

Hope you had a nice vacation Mike.
Will look into the progress inconsistency. I see that it was like that before the fix but should be easy enough to align.
I'll probably release 1.7.0 this weekend.

@yoavniran
Copy link
Collaborator

actually, the inconsistency isnt related to the mock-sender specifically.
In this case I wont be changing it at this point, it may impact too many existing implementations.
Something for v2 perhaps :)

@yoavniran
Copy link
Collaborator

release 1.7.0 is now available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working completed
Projects
None yet
Development

No branches or pull requests

2 participants