Overwritting initialize without calling prototype
Opened this issue · 9 comments
Since you're overwriting the initialize function should add something like bookshelf.Model.prototype.call(this) so it doesn't conflict with other plugins that might also be using the initialize function
I'll write a test and fix it tomorrow. Can you give me an example of a plugin that doesn't work so I can test that as well as use it in the test?
Here's a plugin: https://github.com/bsiddiqui/bookshelf-modelbase
If you add the bookshelf-modelbase plugin before adding bookshelf-scopes, bookshelf-scopes overwrites the initialize function. Calling bookshelf-scopes first, however, works because bookshelf-modelbase call's the prototype's initialize function here
Ok I'll take a look tomorrow, shouldn't be hard to fix. Thanks for pointing this out!
Ok @bsiddiqui I looked into this and I think this is because your module isn't implementing plugins correctly. If you look at your examples you are using the model base that comes back from your plugin add. You should still be using the bookshelf.Model.extend. If you change the last line of your model to not return model but to set bookshelf.Model = model; it should fix it.
We use many different plugins with our bookshelf and this module was the only one that had an issue with it. I'll send you a pull request.
Yeah I've been meaning to change that but I believe I also tested with bookshelf-paranoia, which correctly uses plugins - you're probably right though!
@jtwebman looked into this and you should be calling the parent's initialize function. Checkout this conversation - I also tested this by including two plugins that both overwrite the initialize() method and the second plugin and it causes issues
bookshelf.plugin(require('plugin1'))
bookshelf.plugin(require('plugin2'))If the first plugin doesn't call the parent's initialize() method, it's ok because bookshelf.Model's initialize() method doesn't do anything. If the first plugin overwrites the initialize() method, like bookshelf-scopes does, now the parent has an initialize() that does something.
If the second plugin doesn't call the parent's initialize() method, it is not ok because it will overwrite the first plugins initialize() method.
To be safe, any plugin that modifies built-in methods with something else should call the parent method (in case the parent method does something useful)
@bsiddiqui ok I'll add the test and make it so for you.
Haha don't do it for me - do it because it's an awesome plugin and wouldn't
want anyone to run into issues unknowingly
This might have been fixed now with @vellotis changes. I move the repo to here if it is still and issue place open a bug there. https://github.com/jtwebman/bookshelf-scopes