dumbmatter / fakeIndexedDB

A pure JS in-memory implementation of the IndexedDB API
Apache License 2.0
562 stars 69 forks source link

fix(#76): use arrow functions syntax for members #77

Closed vitonsky closed 3 months ago

vitonsky commented 1 year ago

Fix a bug https://github.com/dumbmatter/fakeIndexedDB/issues/76

dumbmatter commented 1 year ago

I think there is some performance penalty for declaring class methods this way, since they are not part of the prototype? So I'm not sure if it makes sense to do this, just to work around a bug in an old version of another library. Also it is kind of confusing to do it just in this file, but not in any other file, because someone could just as easily access any class method with some other this. Is an old version of idb so special that it deserves a workaround? idk

vitonsky commented 1 year ago

I'm not sure performance penalties you mean. I think performance changes for this refactoring are not significant, but if you have another information, share link please.

I agree about code consistency, so it would be good if we fix another classes, but i would do it in another PR.

As library owner, i think we have to fix this problem not just for special version of IDB, but for the world at all. When users of library found some problems with integrations with one library today, they'll find similar problems tomorrow with another libraries or in their own code.

The problem is - not the same behavior as original IDB, so it may generate unpredictable number of problems. I've fixed it locally as yarn patch on the one project, but another project right now can't use fakeIndexedDB because can't make correct test and used npm that is not support local patches

dumbmatter commented 1 year ago

Performance in the sense that a normal class method is stored only once in memory for all instances, whereas this will be stored once per instance.

That being said, you are right that correctness is the main goal. Can you help me understand that a little? Maybe write a unit test to show the behavior? I tried doing something like this:

request.onupgradeneeded = (e) => {
    const db = e.target.result;
    const names = db.objectStoreNames;
    console.log(names);
    const contains = names.contains;
    console.log(contains("foo"));
};

And that does indeed fail in fake-indexeddb due to this issue (haha get it, "this issue"), but it also fails when I run it with real IndexedDB. I know you posted a line of code from an old version of idb in #76 but I don't understand what the problem is exactly.