-
Notifications
You must be signed in to change notification settings - Fork 459
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
fix: don't modify the passed options object #559
Conversation
LGTM, thanks Robin! |
@@ -10,10 +10,12 @@ var corsHeaders = require('hapi-cors-headers') | |||
var hoodieServer = require('@hoodie/server').register | |||
var log = require('npmlog') | |||
var PouchDB = require('pouchdb-core') | |||
var cloneDeep = require('lodash').cloneDeep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn’t we decide on full loadash
on the server instead of single functions?
No blocker tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn’t we decide on full loadash on the server instead of single functions?
I've adapted from here:
Line 3 in 7a98915
var pick = require('lodash').pick |
But yeah, found out about the CODING_STYLE.md
now, will create another PR that fixes this inside of server/index.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the full lodash. the alternative would have been be require('lodash/cloneDeep')
LGTM |
So in this project we're registering multiple servers as to Hapi, kinda like this:
the problem is, due to hoodie server's behaviour here,
otherEndpoint
never sees what's inoptions.db
and will always have an empty object.This PR fixes this behaviour by cloning the options object before doing anything on it.