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

support for multi-level indicators and JSON #5

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sylvainbx
Copy link

This changes add the following features:

  • ability to use multi-level indicators, eg.
    var opts = {
      modelArray: persons,
      storeWhere: "books.author",
      arrayPop: true,
      mongooseModel: Book,
      idField: 'contacts.author'
    };
  // information will be retrieved from Book.contacts.author and will be store in Person.books.author
  // previously it was not possible to use more than a 1-level indicator (ie. 'books' not 'books.author')
  • ability to convert directly the results to JSON, usefull in Express.js, eg.
  // this in an Express.js controller for a JSON API
  const reversePopulateForJson = require('mongoose-reverse-populate').reversePopulateForJson;

  module.exports.index = function(req, res, next) {
    User.find({}, 'firstName lastName').exec(function(err, users) {
      if (err) return next(err);

      var opts = { ... };
      // here if we had used the normal reversePopulate(), the populated fields won't had appeared
      // in the resulting JSON
      reversePopulateForJson(opts, function(err, populatedUsers) {
        if (err) return next(err);
        res.json(populatedUsers);
      });
    });
  };

This also introduce a breaking change:
var reversePopulate = require('../services/mongoose-reverse-populate');
MUST now be written like this:
var reversePopulate = require('../services/mongoose-reverse-populate').reversePopulate;

@davidbanham davidbanham assigned s-taylor and davidbanham and unassigned s-taylor Mar 20, 2016
@davidbanham
Copy link

Thanks for the PR, Sylvain! Apologies it's taken us a little while to get back to you. We need to set up a better notification system here. We'll be much more responsive in future, I promise.

I'd like to see some tests for the new reversePopulateForJson function before we look at merging it. Seeing a test would also help me better understand the intended purpose of the function, I think.

Are you willing to add a test for this new function to the suite? If there's any part of the existing test code that is unclear or you're uncertain of please let me know and I'll be happy to talk you through it.

@sylvainbx
Copy link
Author

Hi, no problem, thanks for the answer!
I'll try to add a test and I keep you updated.

@davidbanham
Copy link

Great, thanks! Let me know if you need anything.

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.

4 participants