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

Reimplement as a plugin for Bookshelf/Knex 0.8.x #7

Merged
merged 11 commits into from
Apr 11, 2016

Conversation

blah238
Copy link
Collaborator

@blah238 blah238 commented Jun 26, 2015

I've rewritten much of this library to work with the current Bookshelf/Knex versions and added several other improvements and simplifications. I think it's a great library that just needed a bit of an update. See the updated README.md for usage as a plugin.

  • Changed constructor signature to require Bookshelf instance as first argument, and made root an optional property of the second options argument
  • Added Manager.plugin() method to be used with Bookshelf.plugin() (exported on main index.js module)
  • Exposes self as manager property on Bookshelf instance when instantiated
  • Replaced Manager.manage() and Manager.manages() with manager.register() to improve compatibility with Bookshelf Registry plugin
  • Removed hooker dependency and manager.debug() method
  • Use NPM devDependencies and peerDependencies instead of dependencies since it is intended to be used as a plugin now
  • Changed test database from MySQL to SQLite for easier testing setup
  • Updated README.md
  • Bumped version to 0.1.0 (not fully backwards-compatible)
  • Add JSHint/JSCS configs

Please let me know if anyone has any questions. Otherwise I'd love to get this merged!

blah238 added 3 commits June 24, 2015 15:14
  - Changed constructor signature to require Bookshelf instance as first argument, and made `root` an optional property of the second `options` argument
  - Added Manager.plugin() method to be used with Bookshelf.plugin() (exported on main index.js module)
  - Exposes self as `manager` property on Bookshelf instance when instantiated
  - Replaced Manager.manage() and Manager.manages() with manager.register() to improve compatibility with Bookshelf Registry plugin
  - Removed `hooker` dependency and manager.debug() method
  - Use NPM devDependencies and peerDependencies instead of dependencies since it is intended to be used as a plugin now
  - Changed test database from MySQL to SQLite for easier testing setup
  - Updated README.md
  - Bumped version to 0.1.0 (not fully backwards-compatible)
  - Add JSHint/JSCS configs
  - Tests were failing because of: bookshelf/bookshelf#744
  - TODO: Update Bookshelf dependency with next official release which should include the fix to the above issue
@ericclemmons
Copy link
Owner

Holy crap, great job @blah238!!!! I can't express how awesome this is :)

@willrstern: Let's pull this into our admin and try some of our complex saves for the final test. I know there were some specific cases there (mostly super-deeply-nested saving of just the diff) that we fine-tuned like crazy.

@blah238
Copy link
Collaborator Author

blah238 commented Jun 27, 2015

@ericclemmons Happy to give back! Thanks for creating it, I think it's super useful.

Let me know if I can help with any additional testing. I agree it would be good to see how it works in complex situations.

@ericclemmons
Copy link
Owner

I haven't heard of the bookshelf.plugin stuff (though I've heard of the "model registry").

This project was actually the linchpin for a complex Angular admin we built in 2014. It all relied on fetching a deep, complex object from the server (thanks Bookshelf!), modifying it, and POSTing the diff between models and somehow creating all new relations, removing old relations, and modifying scalar props (thanks Bookshelf Manager!)

Unfortunately, reproducing edge cases here was difficult. And there's 1 edge-case we know of, but I'd rather fix that in your new version :)

@blah238
Copy link
Collaborator Author

blah238 commented Jun 27, 2015

The Bookshelf.plugin thing isn't well documented but this comment helped explain it for me: bookshelf/bookshelf#809 (comment)

I've got some similar use cases -- also using this for an admin-panel style Angular app, where I need to attach and detach related records (e.g. roles and permissions) from users and roles, among other things.

I also have some other ideas I'd like to try out, such as limiting the depth of nested updates, limiting to attaching/detaching vs. create/update/deleting, etc.

@blah238
Copy link
Collaborator Author

blah238 commented Jun 29, 2015

Added transaction support (yippee!) -- I think this was sorely needed since a lot can go wrong during one of those complex nested saves, and rolling back on error is the only surefire way to ensure data consistency. Note this is completely optional.

@ericclemmons
Copy link
Owner

Holy crap!

@blah238
Copy link
Collaborator Author

blah238 commented Jun 29, 2015

😁

Will need to be tested with real data of course, but so far is working well in my Angular/Sails app.

We could probably also use the options object for other things like limiting nesting depth or the allowable actions, relations, etc.

blah238 added 5 commits June 30, 2015 12:58
…he relation name and the foreign key name are the same
…present but set to null

  - Fix belongsTo foreign keys not being set to null if a related attribute is present but set to null
@ErisDS
Copy link

ErisDS commented Jul 6, 2015

I'd love to test this plugin out with Ghost, but I'm not 100% on how to use it along with the registry plugin. The README says:

Note: Also compatible with models registered with the Bookshelf Registry plugin.

Does that mean that if the registry plugin is in use, there is no need to register the models at all?

@blah238
Copy link
Collaborator Author

blah238 commented Jul 6, 2015

@ErisDS

I'd love to test this plugin out with Ghost, but I'm not 100% on how to use it along with the registry plugin.

I haven't used Ghost personally, so not sure if it will work. It looks like they are on a slightly older version of Bookshelf/Knex (0.7.x) which I haven't tested this with. If you do run into issues let me know because I think it would be good to be compatible with Ghost.

Does that mean that if the registry plugin is in use, there is no need to register the models at all?

No you still need to register the models one way or another. All that means is if you are already using the registry plugin, this version of the plugin will work with any models you register with the registry plugin. So it should just drop in with any code that is already using the registry plugin. But you still need to register the models at some point, either with the registry plugin, or with this plugin (or mix and match if you so choose).

Hope that helps, let me know if it's still unclear.

I do think it would be nice to have an option that automatically registers all the models in the root directory during initialization, however. Probably even make it the default, but still allow them to be "lazy-loaded" if disabled. In that case I think we would also need an option to specify a "base" model that gets registered first in case the others depend on it.

This is basically what I do currently in my application (only registering using the registry plugin):

1.) Initialize Bookshelf/Knex
2.) call bookshelf.plugin('bookshelf-manager') without the options argument
3.) Register my "BaseModel" model with the registry plugin
4.) Loop over the files in my app's models directory, excluding the BaseModel above, and register each with the registry plugin.

We could likely simplify that workflow since it's probably a common scenario. @ericclemmons please let me know if you have any thoughts on this.

@jamesdixon
Copy link

Any plans to merge this in?

@ericclemmons
Copy link
Owner

@willrstern Can you pull this into the admin & confirm that everything's still good? This would be a pretty remarkable improvement to what we have now...

@vellotis
Copy link

@blah238 @ericclemmons ping? What is the status of this merge?

@blah238 blah238 merged commit d0f29a7 into ericclemmons:master Apr 11, 2016
@blah238
Copy link
Collaborator Author

blah238 commented Apr 11, 2016

@jamesdixon @vellotis @ericclemmons Sorry for taking so long, but I've just merged this as-is, and released on NPM as version 0.1.0. Please let me know if you run into any issues. It's been a long time since I've looked at this code, although we do use it in production without issue.

@blah238
Copy link
Collaborator Author

blah238 commented Apr 11, 2016

I goofed and put v0.10.0 instead of v0.1.0 for the tag/release name, should be fixed now.

@vellotis
Copy link

@blah238 Regards! Great to hear that it is used in the production. We are currently trying to find a solution for multiple problems that could be solved with this library.

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.

5 participants