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

Fix Multiple Image Blob Export #2094

Merged
merged 6 commits into from
Nov 13, 2024

Conversation

chrigglesby
Copy link
Contributor

@chrigglesby chrigglesby commented Nov 13, 2024

Hello there, I'm the developer of noto.ooo and have been enjoying using Dexie and dexie-export-import as part of this project!

I've re-discovered a bug that I believe surfaced in this pull request.
With hopes of fixing this for my own project and to give a little back to this fantastic library I've done my best to put together the following pull request.

Case: When storing multiple Blobs with Dexie, exporting the database with dexie-export-import, then importing that exported database.

Expected Behaviour: I see each of my images displayed.

Actual Behaviour: I see the first of my many images repeated for as many images as there are.

Reproduction of this Bug

I noticed this issue with a new feature I'm developing where images are stored and exported/imported with Dexie and dexie-export-import.

In order to make the issue clearly demonstrable I've put together a sample project:

Reproduction Steps with Sample Project

Step Screenshot
Go to Sample Project App image
Save the provided sample images image
Upload and Add each of the sample images image
image
image
image
Now there are three distinct images in the database, they are rendered on the page (separately from the sample images) image
Export and Save image
Import the Exported file image
You'll see the issue - After importing, the rendered images now show only the first of the three images image

Fix Approach

As I mentioned, I searched for an existing issue for this and found this existing and closed issue. I was curious if this fix worked so pasted it directly into my project's (noto) node_modules/dexie-export-import/dist/dexie-export-import.mjs. Where I had been exporting and importing a large amount of images and being perplexed as to why the same image was repeated for every single one, after applying this patch and exporting/importing again I could verify that my images were displaying as I expected.

Updating the Test

I noticed the other PR was closed due a test failing so thought I had better try and address that.

  • I added more blobs to the test to see that multiple blobs will induce the bug
  • I then applied the fix and see that the tests pass

Testing

I've tested that this fix solves my outlined problem, exporting the same image over and over.

Test Case Anecdotes

  • Initially copy pasting the fix into my node_modules
  • Checking out Dexie/dexie-export-import and symlinking to my project (noto)
    • without fix, bug occurs
    • apply fix, build, see expected results
  • Building a sample bug reproduction project, symlinking to node_modules
    • without fix, bug occurs
    • apply fix, build, see expected results

Closing

Thanks for your continued work and passion on this incredibly valuable library - love it! ❤️
Although I'm a touch out of my depth diving into this unknown codebase, I'm motivated to get this issue fixed and have done my best to make a solid pull request to that end.

@dfahlander
Copy link
Collaborator

Thank you a lot for this effort!

Still there are 2 existing tests that are failing in the edge-cases test:

Looking at the code in tson.ts, I think it looks like the missing part is not to uncomment blobsToAwait = []; but rather to also add blobsToAwaitPos = 0 the line after.

Could you try this? Otherwise I'll see if I could get some time in the weekend to try it.

@chrigglesby
Copy link
Contributor Author

Thanks so much for your prompt response!

Curious that the tests fail, I have them passing consistently on my local machine running npm run test and pnpm test.
How can I trigger the GitHub pipeline, is it automatic?

Never mind, I have made the change you suggested, which indeed makes far more sense than commenting the line out - tested this again with my sample project and have confirmed the bug still fixed.

Thanks!

@dfahlander
Copy link
Collaborator

Thanks, it did the trick! Now one test on dexie is failing but that's a bit strange since this code doesn't change anything there. It's probably something else, I'll try to investigate it.

To run the tests for this addon locally, you need to:

  1. build dexie
  2. cd into the addon and do pnpm test there
pnpm install
pnpm build
cd addons/dexie-export-import
pnpm install
pnpm test

@dfahlander
Copy link
Collaborator

Yes, I verified that the failing test is a new unit test (4 weeks old) that has some timing dependencies and can fail depending on the speed of the hardware. I will merge this PR now and deal with the failing test in another PR.

@dfahlander dfahlander merged commit 8e55011 into dexie:master Nov 13, 2024
4 of 5 checks passed
@chrigglesby
Copy link
Contributor Author

Wonderful - thanks so much for your help! ✨

@dfahlander
Copy link
Collaborator

dfahlander commented Nov 13, 2024

Thank you too 👍 It's released on npm now.
Release notes for this will come in the next dexie release.

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.

2 participants