tomitrescak / apollo-passport-mongodb

MongoDB Native driver for passport
MIT License
0 stars 1 forks source link

Some initial comments #1

Open gadicc opened 8 years ago

gadicc commented 8 years ago

Firstly I feel I have to repeat what I wrote in https://github.com/apollo-passport/apollo-passport/issues/1#issuecomment-241333184 :)

@tomitrescak!! awesome!! that was so fast :) and circleCI and coveralls all set up, amazing! top form :clap:

On the whole looks fantastic. A few very minor comments:

There were a few places where I guess you thought some choices were my own for styling, but were actually specific to rethinkdb:

Config related stuff; I screwed this up, but it's not actually used anywhere yet. I'll let you know of final implementation (e.g. shouldn't destructure data since the id field will overlap, doesn't update an existing config key, etc).

In assertUserEmailData, is there any way to make the update atomic? I can foresee the (rare) situation where an email address is removed between the find and update, resulting in the wrong index being updated. Also, you update the existing record whereas I replace it... Updating is probably better but let me put some more thought into it (assert in the name for me implied an exact match, and was intended to keep functionality very basic; but updates would definitely super useful).

For createUser, Meteor users will love this but I think that the default should be to use Mongo Object Ids. Maybe something like:

function defaultIDGenerator() {
  return new mongodb.ObjectID();
}

overriden with

import Random from 'meteor-random';

new MongoDriver(db, {
  IDGenerator: Random.id
});

Re _id mapping, sorry, I got confused with mapUserToServiceData. I think for now let's implement something like:

id(doc) {
    return doc._id;
}

and see how we go with it.

Thanks again for the quick work on this! Great to have a mongodb driver before the project is even officially announced :) :)

gadicc commented 8 years ago

Oh and jsdoc failed on my system too, must be something in the code somewhere (I've seen it break on some other ES6 stuff, and I used to a plugin to strip out e.g. async/await).

In theory there's no need for you to even maintain this, since the docs should be identical for any DB driver. But I'd be interested to see where it's breaking anyway.

tomitrescak commented 8 years ago

Gadi, I could not make this work with a ready app as you load drivers automatically from the 'db/' directory. How do you foresee to include all custom drivers?

I really like textual ids as they work really well for optimistic updates. Do I understand well that you want to let users choose how to generate their ids?

To be honest I wrote the driver and I have a bit issues to set this up in a live app ;)

gadicc commented 8 years ago

Oh wow that's really old code... are you running from npm? See if you can run from master via npm link (I think I explained how in the CONTRIBUTING if you're not familiar with it). Otherwise I can publish something now, things are just a bit messy :> I refer to apollo-passport and react; local is stable and published.

As for how to load DB drivers currently, you pass in an instance via the db option to ApolloPassport, see the README on apollo-passport/master.

Let me know if anything else doesn't work; to my knowledge you're the first person besides me using this :>

gadicc commented 8 years ago

Oh, and local doesn't (currently) recognize Meteor stored passwords, but it wouldn't be too difficult to add that or use the same format (for the newer bcrypt stored passwords at least; no plans to go back to srp too).

tomitrescak commented 8 years ago

I must be looking somewhere in the incorrect place. Your master in Apollo-passport has a last modification 20 days ago and it still uses directory recognition for drivers. Did you push your changes recently?

gadicc commented 8 years ago

Where / which file?

gadicc commented 8 years ago

There's a saturn-starter and meteor-starter now.

gadicc commented 8 years ago

Oh and I also wanted to say how awesome it was that apollo-passport-mongodb worked right out the box :) Good job! Will check out apollo-modules tomorrow.

tomitrescak commented 8 years ago

Ah, no problemo ... ever since I worked with that repo I know how to setup coveralls and CI with CirceCI so now all my projects are very fancy so thanks for that. Also I am reusing your DB testing functionality so it was veeeeeeeery beneficial both ways ;)