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

Model class should use prototype rather than assigning new methods to each model in the constructor. #773

Open
insidewhy opened this issue Mar 4, 2017 · 3 comments

Comments

@insidewhy
Copy link

Using prototype has a number of advantages, but mostly the current method being used to populate methods per instance is very bad for performance and memory usage.

@insidewhy
Copy link
Author

It's the same for other classes too, like AggregateFunctions etc. In fact the only place this code is using prototype is in the drivers. 👎

@dxg
Copy link
Collaborator

dxg commented Apr 7, 2017

Probably has something todo with the fact we're using getters and setters.
Perhaps when this package was originally written JS didn't allow getters/setters on prototypes? Not sure.

Would be interested to see if it's possible to change it. Feel free to submit some code.

@insidewhy
Copy link
Author

Even if that were the case it's still a terrible reason for making every non-property instantiated in the constructor. This is some of the worst performing code I've used in ages, it was a massive mistake for my previous client to use it. No one should use orm2.

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

No branches or pull requests

2 participants