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(#76): use arrow functions syntax for members #77

Closed
wants to merge 1 commit into from

Conversation

vitonsky
Copy link

@vitonsky vitonsky commented Nov 9, 2022

Fix a bug #76

@dumbmatter
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

dumbmatter commented Nov 16, 2022

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.

@dumbmatter dumbmatter closed this May 20, 2024
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.

None yet

2 participants