feat(config): Mongoose 4.11 upgrade #1818

mleanos · 2017-07-15T04:07:50Z

Upgrades Mongoose to ~~4.11.1~~ 4.11.3

Updates Mongoose connection implementation to accommodate deprecated
features & connection options.

Also, updates the Gulp Mocha tasks to reflect changes to the Mongoose
implementation.

Fixes tests to get the database from the existing Mongoose singleton.

Derives from changes in #1816

mleanos · 2017-07-15T04:41:22Z

I started on a bigger refactor of the Mongoose and Express app implementation. However, I scaled it back to just satisfy the Mongoose upgrade. We can discuss a bigger refactor in a separate issue and/or PR.

Regarding the { user: '', pass: '' } connection options, there is work being done to add back support of this feature.

lirantal · 2017-07-17T05:30:03Z

@mleanos are you synced with @simison? is this a superset of his PR?

simison

Left a few notes but otherwise looks really good!

Happy you figured out that issue with Mocha.

simison · 2017-07-17T08:52:16Z

config/lib/mongoose.js

-  });
+      if (cb) cb(connection.db);
+    })
+    .catch((err) => {


some ES6 here ;-)

Do you think it's ok?

Node.js >=6.4.0 can handle handle arrow functions. So why not?

I'd love to see bigger discussion about this, actually. This topic just came up at Trustroots so I might open an issue here just to get some food for thought.

I don't think it's a good idea to introduce mixed coding style in PR like this.

It's more annoying to me to see mixed style rather than old school JS.

I'd love to see meanjs move on to ES6 but it might be better done with some strategy and thought and build tools (eslint) could probably be upgraded first.

WDYT?

Everything you just said seems very reasonable to me. I'd like to discuss the introduction of ES6 as well. I think it started becoming a habit for me, since I'm using some features like arrow functions in my projects.

I'll remove my ES6 usage from this PR.

simison · 2017-07-17T08:54:29Z

gulpfile.js

-        mongoose.disconnect(function () {
-          done(error);
-        });
+        mongooseService.disconnect(done);


I almost did this myself as well, but that error being passed to gulp's done isn't error from mongoose disconnect. It's the error object from Mocha.

Error thrown from disconnecting probably doesn't need to fail the whole gulp task? Dunno.

Perhaps that variable needs renaming so that it would be clear what's happening here?

Yea, good points. It did occur to me, for a moment, about the error needing to be passed to the done. Then I think I just overlooked it. I'll change it to pass the Mocha error to the done callback. I guess we can just log the error to the console, when the disconnect fails; it would be a rare error in this case.

The name error seems fine to me. It's clear what's going on since it's right there.

simison · 2017-07-17T08:55:19Z

gulpfile.js

@@ -362,16 +359,17 @@ gulp.task('karma:coverage', function(done) {
 // Drops the MongoDB database, used in e2e testing
 gulp.task('dropdb', function (done) {
  // Use mongoose configuration
-  var mongoose = require('./config/lib/mongoose.js');
+  var mongooseService = require('./config/lib/mongoose');


Same as earlier:

Is there any reason why this require is inside the task and not on top of the gulp file with all the other dependencies?

If our Mongoose service is only being used in a couple places, why require it at the global scope?

My understanding of how require works, along with it's memory use, isn't great so maybe it doesn't matter.

Then I don't know more than you do. :-) I was just curious, I guess for now we can trust someone knew what they were doing when they placed it inside the task.

Amos added it way back in 0.4 release branch.
We can move it at the top if it makes sense now, but I think when Amos added it on 0.4 the mongoose.js worked differently from today, as in, just requiring it may have opened the database connection or instantiate models so that would explain why he would have wanted to limit it to the specific gulp tasks.

simison · 2017-07-17T08:55:32Z

gulpfile.js

@@ -277,15 +277,15 @@ gulp.task('templatecache', function () {

 // Mocha tests task
 gulp.task('mocha', function (done) {
-  // Open mongoose connections
-  var mongoose = require('./config/lib/mongoose.js');
+  var mongooseService = require('./config/lib/mongoose');


Is there any reason why this require is inside the task and not on top of the gulp file with all the other dependencies?

simison · 2017-07-17T08:56:41Z

modules/articles/tests/server/admin.article.server.routes.tests.js

@@ -23,7 +23,7 @@ var app,
 describe('Article Admin CRUD tests', function () {
  before(function (done) {
    // Get application
-    app = express.init(mongoose);
+    app = express.init(mongoose.connection.db);


This <3, I just couldn't figure out what was wrong before.

This was a source of confusion to me too. I didn't see how the line above would work by just passing mongoose, but eventually it made sense based on this: https://github.com/meanjs/mean/blob/master/config/lib/mongoose.js#L27. db is a promise, no?

If these changes get merged then it's a moot point, I guess. Ideally, we need to refactor the Express & Mongoose implementations. As it stands, after we make the first connection, we're heavily relying on the fact that Mongoose is a singleton. This could be confusing, and probably not the best SOC design.

Yup!

I guess https://github.com/lirantal/Riess.js might have quite a lot of it thought through already.

And that said I think it's a good idea to keep this to minimum to just get Mongoose upgraded and leave refactoring to a separate PR.

Agreed 👍

Yep. And indeed the returned value of the mongoose.js line 27 was just the db object being resolved with.

mleanos · 2017-07-18T00:12:28Z

@lirantal Mikael and I discussed in his other PR that I would be taking this over. Once I get a couple things clarified from the review comments, I'll rebase and make the necessary changes.

mleanos · 2017-07-18T01:44:44Z

@lirantal @simison Final review?

Based on Mikael's review:

Removed arrow function use
Added back done(error) to the Mocha Gulp task to return error thrown by Mocha rather than the mongooseService.disconnect callback.

simison · 2017-07-18T06:48:14Z

Looks good to me now!

simison

Noticed one more minor thing.

simison · 2017-07-19T08:43:03Z

config/env/development.js

-      user: '',
-      pass: ''
-    },
+    options: {},


Could you remove these also from the example config file?

https://github.com/meanjs/mean/blob/master/config/env/local.example.js#L23-L26

Sure thing.

mleanos · 2017-07-19T18:01:48Z

@simison @lirantal I've rebased and squashed my commits.

I also removed the options from https://github.com/meanjs/mean/blob/master/config/env/default.js#L10, that I had previously added in a commit on this branch. I just remembered that the default env config doesn't need the options, since they are being set in the individual env configs (i.e. development, production, and test).

lirantal · 2017-07-19T21:40:51Z

modules/articles/tests/server/article.server.routes.tests.js

@@ -119,7 +119,7 @@ describe('Article CRUD tests', function () {
    // Save the article
    articleObj.save(function () {
      // Request articles
-      request(app).get('/api/articles')
+      agent.get('/api/articles')


nice catch @mleanos

lirantal · 2017-07-19T21:43:40Z

modules/users/server/config/users.server.config.js

@@ -11,7 +11,7 @@ var passport = require('passport'),
 /**
 * Module init function
 */
-module.exports = function (app, db) {
+module.exports = function (app) {


this could potentially break vertical modules that rely on it.
even with the changes in this PR for config/lib/mongoose.js how would one be able to access the db instance?

the other alternative I can think of is if we set app.local.db or something like that which would expose the db connection where app is too.

WDYT?

I like that, sounds simple and straightforward. I didn't like at all trying to follow where db is going around trough all those callbacks.

That said, this could also wait for more thorough restructuring of mongoose/db stuff as with promises etc.

In leaning towards agreeing with 'this can wait' but I wonder what projects built based on master version will do if db is now not available for them and they wish to stay up to date...

We should note the potential breaking change in the CHANGELOG.

Anyone merging from master should be aware of the changes, and it seems like it would be easy for them to add back the db in their exports where they rely on it. Adding app.local.db wouldn't be a difficult task either.

I'm a bit skeptical that many use cases would arise that would require someone to rely on accessing the db instance from server-side config/controllers.

I'm not opposed to adding db back to all the callbacks though.

yeah which is why I'm not totally against removing it.
so let's keep this PR as is and see how the mongodb refactoring continues on. I'm ok with that.

Ok. I'm fine with that as well.

mleanos · 2017-07-22T04:07:42Z

@lirantal Are you still waiting on changes before you can approve this?

lirantal · 2017-07-22T05:37:28Z

Nope, I'm good.

simison

Found another small issue.

simison · 2017-07-25T09:03:35Z

config/lib/mongoose.js

@@ -24,25 +25,28 @@ module.exports.connect = function (cb) {



Remove var _this = this; as it's never used (weird how eslint didn't warn about this?)

Removed. The ESLint rule no-unused-vars is turned off, probably because there are a lot of ESLint errors generated from this rule in our tests and other places :/ That can be addressed in a separate PR.

simison · 2017-07-25T09:06:19Z

config/lib/mongoose.js

-      if (cb) cb(db);
-    }
-  });
+      if (cb) cb(connection.db);


Personally I prefer using callback instead of cb just to keep it all the same and I dislike abbreviations in coding (just needs extra thinking when reading it), but this isn't a biggie. :-)

Upgrades Mongoose to 4.11.1 Updates Mongoose connection implementation to accommodate deprecated features & connection options. Also, updates the Gulp Mocha tasks to reflect changes to the Mongoose implementation. Fixes tests to get the database from the existing Mongoose singleton. Derives from changes in meanjs#1816 Closes meanjs#1814

mleanos · 2017-07-26T02:42:28Z

@simison I addressed the changes requested in your last review. LGTY?

@lirantal If everything looks good now, I think this is ready to be merged.

lirantal · 2017-07-26T05:19:33Z

Yep, I'm good.

simison · 2017-07-26T15:26:42Z

LGTM! Feel free to merge.

feat(config): Mongoose 4.11 upgrade (meanjs#1818)

Upgrades Mongoose to 4.11.1 Updates Mongoose connection implementation to accommodate deprecated features & connection options. Also, updates the Gulp Mocha tasks to reflect changes to the Mongoose implementation. Fixes tests to get the database from the existing Mongoose singleton. Derives from changes in meanjs#1816 Closes meanjs#1814

mleanos force-pushed the feature/mongoose-upgrade branch from 1eb79a1 to 1c6faff Compare July 15, 2017 04:36

mleanos requested a review from lirantal July 15, 2017 04:58

mleanos assigned lirantal Jul 15, 2017

mleanos added need:review+1 platform:build platform:mongodb labels Jul 15, 2017

simison suggested changes Jul 17, 2017

View reviewed changes

simison marked this as a duplicate of #1816 Jul 17, 2017

simison mentioned this pull request Jul 17, 2017

[WIP] Update Mongoose #1816

Closed

mleanos force-pushed the feature/mongoose-upgrade branch from 1c6faff to b4c3565 Compare July 18, 2017 01:19

mleanos mentioned this pull request Jul 18, 2017

feat(config): Mongo Seed 2.0 #1808

Merged

simison approved these changes Jul 18, 2017

View reviewed changes

simison reviewed Jul 19, 2017

View reviewed changes

mleanos force-pushed the feature/mongoose-upgrade branch from b4c3565 to 1ed438f Compare July 19, 2017 17:52

lirantal reviewed Jul 19, 2017

View reviewed changes

lirantal suggested changes Jul 19, 2017

View reviewed changes

lirantal approved these changes Jul 22, 2017

View reviewed changes

simison suggested changes Jul 25, 2017

View reviewed changes

simison reviewed Jul 25, 2017

View reviewed changes

mleanos force-pushed the feature/mongoose-upgrade branch from 1ed438f to 8940173 Compare July 26, 2017 02:36

simison approved these changes Jul 26, 2017

View reviewed changes

mleanos merged commit dc880eb into meanjs:master Jul 27, 2017

GulajavaMinistudio added a commit to GulajavaMinistudio/mean that referenced this pull request Jul 28, 2017

Merge pull request #14 from meanjs/master

3e0d5f1

feat(config): Mongoose 4.11 upgrade (meanjs#1818)

		@@ -24,25 +25,28 @@ module.exports.connect = function (cb) {

feat(config): Mongoose 4.11 upgrade #1818

feat(config): Mongoose 4.11 upgrade #1818

Conversation

mleanos commented Jul 15, 2017 • edited Loading

mleanos commented Jul 15, 2017

lirantal commented Jul 17, 2017

simison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

simison Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Jul 18, 2017

mleanos commented Jul 18, 2017

simison commented Jul 18, 2017

simison left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Jul 22, 2017

lirantal commented Jul 22, 2017

simison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Jul 26, 2017

lirantal commented Jul 26, 2017

simison commented Jul 26, 2017

mleanos commented Jul 15, 2017 •

edited

Loading

mleanos Jul 17, 2017 •

edited

Loading

mleanos Jul 18, 2017 •

edited

Loading

simison Jul 17, 2017 •

edited

Loading

simison Jul 17, 2017 •

edited

Loading

simison Jul 17, 2017 •

edited

Loading

simison left a comment •

edited

Loading