search

remotestorage / remotestorage.js

⬡ JavaScript client library for integrating remoteStorage in apps
https://remotestoragejs.readthedocs.io
MIT License
2.3k stars 141 forks source link

Fix ItemsMap doesn't match real object #1279

Closed darkdread closed 1 year ago

darkdread commented 1 year ago

Multiple reads after tasks have been fetched causes folder index to be misaligned. This fix forces fetched tasks to be queued, ensuring that task handlers finish committing their changes before running the next task handler.

fixes #1187

raucao commented 1 year ago

@darkdread Welcome, and thank you for the proposed fix! It makes sense to me at first glance, and this bug surely is a rather critical show stopper.

Since you seem to have nailed the bug down to the actual problem there, I'm wondering if you could add a test case that demonstrates the bug and that this implementation fixes it? That would make it both easier to review as well as bulletproof against future regressions.

The testing setup is pretty straight-forward, and even though we're in the process of porting everything from Jaribu to Mocha/Chai (and TS), it's still fine to add new cases to the existing suites.

raucao commented 1 year ago

Great!

@darkdread Just to confirm with you: the first test case added is a missing case and supposed to be green on master (it is for me at least), while the second one is testing the bugfix, correct?

darkdread commented 1 year ago

Great!

@darkdread Just to confirm with you: the first test case added is a missing case and supposed to be green on master (it is for me at least), while the second one is testing the bugfix, correct?

In order to simulate the bug, you have to emulate the cloning properties of localStorage and indexeddb for inmemorystorage. I believe the current getNodes implementation of inmemorystorage is a bug; it should behave the same way as its other counterparts.

The first test case after applying the inmemorystorage fix should fail on master, while the second test case should pass.

On a side note, should I open a bug for the fix I applied?

raucao commented 1 year ago

Ok, thanks. Confirmed.

On a side note, should I open a bug for the fix I applied?

Yes, I think that's a good idea.

DougReeder commented 1 year ago

FWIW, I'm not familiar with this part of the code to knowledgeably review it. Thanks, everyone, for your work!