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

Allow multiple instances of keystone #1777

Closed
30 tasks done
creynders opened this issue Nov 9, 2015 · 12 comments
Closed
30 tasks done

Allow multiple instances of keystone #1777

creynders opened this issue Nov 9, 2015 · 12 comments

Comments

@creynders
Copy link
Contributor

Continuation of #1775. Let's track our progress here, @snowkeeper @JedWatson

So for the time being all that needs to happen is for instance in List, instead of var keystone=require('../'); we want to make it like this (as you already suggested @jed):

  module.exports = function(keystone){
      //do something with    keystone
  } 

My suggestion for now is that we ​_do_​ leave the circular dependencies. We rewrite all the keystone dependents to use a closure as above.

I think these are the files that need to be updated and obviously also wherever they are required, not including tests:

  • admin/server/api/cloudinary.js
  • admin/server/api/counts.js
  • admin/server/api/download.js ???
  • admin/server/api/item/delete.js
  • admin/server/api/item/get.js
  • admin/server/api/list/delete.js
  • admin/server/api/list/download.js
  • admin/server/api/s3.js
  • admin/server/api/session/signin.js
  • admin/server/api/session/signout.js
  • admin/server/routes/home.js
  • admin/server/routes/item.js
  • admin/server/routes/list.js
  • admin/server/routes/signout.js
  • fields/types/cloudinaryimage/CloudinaryImageType.js
  • fields/types/cloudinaryimages/CloudinaryImagesType.js
  • fields/types/location/LocationType.js
  • lib/content/index.js
  • lib/content/page.js
  • lib/email.js
  • lib/list.js
  • lib/list/register.js
  • lib/list/relationship.js
  • lib/schemaPlugins/history.js
  • lib/schemaPlugins/methods/getRelated.js
  • lib/schemaPlugins/track.js
  • lib/session.js
  • lib/updateHandler.js
  • lib/updates.js
  • lib/view.js

All changes should be pushed to the prototype branch. Also, don't forget to rebase on top of master!

IMPORTANT: wherever a file only uses a dependency of keystone, please inject the dependency directly, if possible. E.g. admin/server/api/counts.js only uses keystone.lists, i.e. I'll be injecting lists instead of keystone.

@creynders creynders added this to the 0.4.0 milestone Nov 9, 2015
@creynders
Copy link
Contributor Author

@JedWatson Any idea where admin/server/api/download is used? Can't seem to find any require calls or references.

@snowkeeper snowkeeper mentioned this issue Nov 10, 2015
@snowkeeper
Copy link
Contributor

I just pushed my changes in #1785

To move through in a timely manner, anything that was a prototype and needed keystone injected got moved to the constructor. Y'all can clean that up if you want.

In testing I needed a new mongoose instance for each new instance so I changed this.mongoose = require('mongoose') to this.mongoose = new mongoose.Mongoose(). Not sure how you want to handle that.

Everything else I have tested works so far. I included test/server.all.test.js. It creates a new instance for each server. Creates httpModel and sslModel in database keystone and socketModel in database test-site.

@creynders
Copy link
Contributor Author

@snowkeeper you're a machine! At a first glance everything looks good. I think the mongoose thing is ok, we just have to keep in mind that people need to be able to overwrite the mongoose instance with their own instance, i.e. when we provide a stop method I think it would be best if we add a keystone.vendors.mongoose which could be used to create new mongoose instances of, or change the line to:

this.set('mongoose', new (this.get('mongoose') || mongoose).Mongoose());

@JedWatson
Copy link
Member

@creynders hmm...

  • keystone.vendor.mongoose
  • keystone.modules.mongoose
  • keystone.packages.mongoose

...?

We could bundle express in there as well. I like it, simplifies the new app creation stuff too.

@creynders
Copy link
Contributor Author

I'd go for vendor since it's a community default name for 3rd party modules. And yes, let's stuff express in there as well!

@creynders
Copy link
Contributor Author

@JedWatson I manually merged #1785, rebased on top of master and fixed conflicts. I think we need to land this one ASAP, since it'll only become harder and harder to integrate it, due to future conflicts.

@creynders
Copy link
Contributor Author

I'll make the vendor stuff a separate PR, once this has landed

@creynders
Copy link
Contributor Author

@JedWatson is there a reason you're withholding this one? Or simply cold feet? ;)

@mxstbr
Copy link
Collaborator

mxstbr commented Mar 16, 2016

Did this ever happen? #1785 got closed...

@JedWatson
Copy link
Member

Unfortunately this got really out of date... I've picked it up in much smaller pieces and am working through the remaining bits. Will tag this issue in my commits, but basically going to do this bit by bit (and ensure everything works with individual commits going forward in master) rather than try and branch then merge a big PR.

@derekmpeterson
Copy link

@JedWatson Has there been any more progress made on this?

@gautamsi
Copy link
Member

Keystone 4 is going in maintenance mode. Expect no major change. see #4913 for details.
Keystone v5 is modular and flexible, see if it helps, if not open issue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants