These are chat archives for Automattic/mongoose

6th
Apr 2018
Can anyone help with this?
Arun Gadag
@arun-awnics
Apr 06 2018 07:22
@nsuchy Why are you using this?
UserSchema.pre('save', (next) => {
    bcrypt.hash(this.password, 10, (err, hash) => {
      if (err) {
        return next(err);
      }
      this.password = hash;
      next();
    });
  });
Just use password instead
Kev
@lineus
Apr 06 2018 08:52
@arun-awnics in middleware functions, this should refer to the document but in this case the arrow function is breaking that.
@nsuchy don't use arrow functions for middleware ( or virtuals, statics, query helpers, or getters/setters ) see the FAQ here ( it's the 4th question down )
consider this example
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:25
@arun-awnics a tutorial told me to use this. I can safely remove it?
@lineus I saw your stackoverflow answer I’ll adjust for your answer once I’m at keyboard. Thank you :smile:
Kev
@lineus
Apr 06 2018 11:30
happy to help @nsuchy !
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:39

@lineus On a side note to call express router I write

const router = express.Router();

The Google JavaScript styleguide has this requirement that a function with an Uppercase letter should only be used as a constructor. Is there a way around this so I can use express router?

Screenshot from last night showing the issue
Kev
@lineus
Apr 06 2018 11:42
that's the thing about style guides and linters. not everyone follows the same rules :)
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:44
I want my code to be compliant though
I agree with most of their style guide
Since my code is linted it needs to be compliant entirely.
Kev
@lineus
Apr 06 2018 11:46
it's not your code that isn't compliant, it's theirs. can't you add an ignore comment to that line?
for your linter
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:47
The linter checks Googles style guide.
Annoyingly it interferes with requiring that function
I’m not sure if there’s a way around it
I don’t want to ignore limiting errors, they’re there to help you.
Kev
@lineus
Apr 06 2018 11:47
// eslint-disable-next-line
what linter are you using?
It has ecmascript 6 enabled in the config.
Kev
@lineus
Apr 06 2018 11:51
from the express issue queue expressjs/express#3058
tl;dr just use new
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:51
Well my config does
So write: const router = new express.Router();
?
Kev
@lineus
Apr 06 2018 11:52
yup :)
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:52
Thanks
Kev
@lineus
Apr 06 2018 11:52
:)
Nathaniel Suchy
@nsuchy
Apr 06 2018 11:53
I want to start writing better JavaScript and using ESLint and a style guide is a good way to start.
The whole semi colon requirement is insane - that said majority rules and I like most of Google style guide so willing to comply :smile:
Kev
@lineus
Apr 06 2018 11:58
I prefer no-semicolons myself. I use the "standard" eslint style for my own code and custom configs when I'm contributing elsewhere.
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:00
Standard is nice - Google has strict requirements for readability and performance. I try to make sure all of my code is linted as I’d have for the quality of my codebases as drop. Humans will always make mistakes - linters help reduce them.
Next up finish learning React 😁😁😁
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:08
@lineus One last thing - do I need to put `'use strict;' at the top of every file or just app.js?
Kev
@lineus
Apr 06 2018 12:09
I put it at the beginning of every file, but honestly that's just a habit from my perl days. I never stopped and thought about it.
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:13
UserSchema.pre('save', function(next) {
  bcrypt.hash(password, 10, (err, hash) => {
    if (err) {
      return next(err);
    }
    password = hash;
    next();
  });
});
And this will work fine without writing this in front of password?
Kev
@lineus
Apr 06 2018 12:14
I think the spec is saying that if you declare it at the top of a module file, it only applies to that module file.
no that won't work. you have to pass in a common function. userSchema.pre('save', function(next) { .. }) instead of the arrow function.
carefully inspect the value of this between the two log statements in my example
you have to specify this to get at the property password in the document, but arrow functions and common functions don't handle this the same.
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:17
So the bcrypt.hash function cannot be an arrow function?
Kev
@lineus
Apr 06 2018 12:18
the arrow function passed into bcrypt is fine
it's the one that you're passing into userSchema.pre that has to be a common function
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:19
Screen Shot 2018-04-06 at 8.18.49 AM.png
Kev
@lineus
Apr 06 2018 12:19
oh, wait, sorry I just caught that in your example just now it's right.
my eyes failed me :)
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:19
See screenshot, even writing like that causes an issue with the linter
And I have to write this.password?
Kev
@lineus
Apr 06 2018 12:21
yes
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:21
which causes a linting error
Kev
@lineus
Apr 06 2018 12:21
you'll have to tell the linter to ignore that line
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:22
I don't want to ignore linting errors, if the line is wrong I need to fix it
Kev
@lineus
Apr 06 2018 12:22
if that wasn't ok, eslint wouldn't give you a way to ignore it
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:22
It's not compliant with the style guide I'mm using
Is it a bug in ESLint / the config itself?
Kev
@lineus
Apr 06 2018 12:25
it's a perfectly valid eslint pattern to ignore a rule. docs here
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:26
I want to comply with all rules across my codebase unless there's a valid reason not to
Is there anyway to do it without using this
Kev
@lineus
Apr 06 2018 12:26
the order of importance is
  1. valid javascript
  2. linter
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:26
As for my question is this a bug with ESLint / the config?
Perhaps with Mongoose.js?
If so I should open an issue
Kev
@lineus
Apr 06 2018 12:28
the only way around this is to never use middleware, virtuals, getters/setters, query helpers, or static/instance methods on schemas.
it's not a bug with mongoose
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:28
Well that's not a possibility - how does Google use scehmas then?
Unless they just don't use mongoose
lol
Going to disable the linter on those lines for no-invalid-this until I can do further research
Going to track down Googlers on Twitter and ask
Kev
@lineus
Apr 06 2018 12:47
google doesn't appear to use mongodb at all in their public repos anyway
so you're right, they probably don't use mongoose.
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:52
I like the whole tableless idea
I just add data to objects as needed
you could spend hours working on a single mysql or postgres table
Kev
@lineus
Apr 06 2018 12:53
I'd be lying if I said I hadn't spent hours working on a mongodb aggregation pipeline :)
Nathaniel Suchy
@nsuchy
Apr 06 2018 12:56
:joy:
I wonder if spending hours to make this codebase more readable was worth it
:thought_balloon:
Kev
@lineus
Apr 06 2018 12:58
you'll know one way or another in a few months or years when you go back to add/fix something
chances are good it was worth it
Nathaniel Suchy
@nsuchy
Apr 06 2018 13:11
A friend of mine runs a gaming forum where they share "ROM hacks" and their database in PHP was bad (I know because I found and fixed multiple critical risk security issues in it for him). I did a complete rewrite in Node.js he likes it so far though right now scrolling down 1000s of lines is painful and it'll only get worse as more routes and features are added.
Best to fix this now
Nathaniel Suchy
@nsuchy
Apr 06 2018 16:29
@lineus interestingly enough Google Cloud Platform only offers managed MySQL and PostgreSQL, MongoDB and Cassandra are not offered
Juha Lindstedt
@pakastin
Apr 06 2018 19:52
MongoDB Atlas is a good deal!
Nathaniel Suchy
@nsuchy
Apr 06 2018 20:49
'use strict';
const express = require('express');
const router = new express.Router();
const hack = require('../models/hack');

router.get('/', (req, res, next) => {
  hack.count().exec( (err, count) => {
      if (err) {
        return next(err);
      }
      const random = Math.floor(Math.random() * count);
      hack.findOne({isDeleted: false}).skip(random).exec((err, result) => {
          if (err) {
            return next(err);
          }
            res.redirect(`/view/${result._id}`);
      });
  });
});

module.exports = router;
I have the above source code. Sometimes MongoDB will return a null object resulting in an application crash. How can I handle this when the error occurs?
App listening on port http://localhost:5000/
events.js:165
      throw er; // Unhandled 'error' event
      ^

TypeError: Cannot read property '_id' of null
    at hack.findOne.skip.exec (/Users/nathanielsuchy/Coding/sm64hackdb/routes/random.js:16:22)
    at /Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mongoose/lib/model.js:3935:16
    at (anonymous function).call (/Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mongoose/lib/query.js:3017:7)
    at Immediate.<anonymous> (/Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mongoose/lib/query.js:1514:14)
    at Immediate.<anonymous> (/Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mquery/lib/utils.js:119:16)
    at runCallback (timers.js:763:18)
    at tryOnImmediate (timers.js:734:5)
    at processImmediate (timers.js:716:5)
Emitted 'error' event at:
    at /Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mongoose/lib/model.js:3937:13
    at (anonymous function).call (/Users/nathanielsuchy/Coding/sm64hackdb/node_modules/mongoose/lib/query.js:3017:7)
    [... lines matching original stack trace ...]
    at processImmediate (timers.js:716:5)
[nodemon] app crashed - waiting for file changes before starting...
Kev
@lineus
Apr 06 2018 20:55
@nsuchy you can wrap that redirect in a test like if (result) { res.redirect(here) } else { res.redirect(there) }
Nathaniel Suchy
@nsuchy
Apr 06 2018 21:17
that fixed it
the weird thing is why did it work before
Kev
@lineus
Apr 06 2018 21:20
actually, looking at the code, since findOne is only ever going to find one, if skip is greater than 0, won't it return null most of the time?
Nathaniel Suchy
@nsuchy
Apr 06 2018 21:21
I think it's an mlab limit
if you send several queries it sometimes returns a blank object
it mainly happens if I load /random multiple times rapidly
On a good note I finally read up on error handling as a result of this and am editing my render() functions to handle errors
:D
probably going to switch isLoggedIn and canEdit to app.locals next up
Kev
@lineus
Apr 06 2018 22:41
@nsuchy take a look at this gist
note that I'm forcing my looped query to find the first doc every time, and even with that constraint, very few of the results are non-null.
the .skip() doesn't effect the query, but the cursor returned from the query.
you can see by the assert statement at the end, I can predict with 100% accuracy the number of queries that are going to set a non-null result, because anytime skip is greater than 1, it's going to kill the result of findOne.
Kev
@lineus
Apr 06 2018 22:53
the api docs normally do have a link to the mongodb cursor.skip page, but there's a bug in the latest release or 2 that resulted in all of the jsdoc param statements not being included/marked down in the api docs. So that should be fixed in 5.0.14 if my PR gets approved.
meant to say anytime skip is greater than 0 ;)
Kev
@lineus
Apr 06 2018 23:17
cursor.skip docs are here the important bit being offset The number of documents to skip in the results set. cursor === results set, and with findOne, will at most only ever be 1 results long.
Kev
@lineus
Apr 06 2018 23:36
here's another gist showing an alternative solution to grabbing one random doc