-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fixes #1417 #1420
Fixes #1417 #1420
Conversation
@@ -647,31 +646,84 @@ function untransformObject(schema, className, mongoObject, isNestedObject = fals | |||
|
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.
why don't we do something like
if (isNestedObject) {
let transformableKeys = ["_id", ...];
Object.keys(mongoObject).forEach((key) => {
if (transformableKeys.indexOf(key) > -1) {
restObject[key] = untransformObject(schema, className, mongoObject[key], true);
} else {
restObject[key] = mongoObject[key];
}
});
return restObject;
}
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.
Yeah I will refactor
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.
maybe move let transformableKeys
some where else, and if we get crazy we could have a structure like:
transformKey: {
key: {
nested: (key, mongoObject, restObject) => {
// to the mongoObjectTransform and assign on restObject
},
default: (key, mongoObject, restObject) => {
// to the mongoObjectTransform and assign on restObject
},
}
}
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.
I went with something simpler. Moving this logic all to the mongo adapter will be a better time to fix it up.
@drew-gross updated the pull request. |
'expiresAt', | ||
'_expiresAt', | ||
]; | ||
if (_.includes(specialKeys, key) && isNestedObject) { |
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.
should switch these two statements around so the if will exit early with the boolean compare rather than a Object key iteration
if(isNestedObject && _.includes(specialKeys, key)) {
// ...
}
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.
Good idea! Thanks.
@drew-gross updated the pull request. |
Hmmm travis isn't picking up this commit.... |
It also missed building #1416 |
Whoops. Guess we better not do a release until we get passing tests. |
meh... |
Current coverage is
|
@@ -647,6 +645,25 @@ function untransformObject(schema, className, mongoObject, isNestedObject = fals | |||
|
|||
var restObject = untransformACL(mongoObject); | |||
for (var key in mongoObject) { | |||
const specialKeys = [ |
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.
should we move that up? I don't think that behaves as a static in Obj-C
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.
If babel is anything like typescript that would just be converted to var specialKeys
So ideally this should be declared outside of the function, or at the very least outside of the loop.
@drew-gross updated the pull request. |
_tombstone: { | ||
_updated_at: "I'm sure people will nest keys like this", | ||
_acl: 7, | ||
_id: {} |
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.
can you throw in a regular key there? (see the other comment)
ebb4ad9
to
3959929
Compare
Passed locally even with regular keys |
@drew-gross updated the pull request. |
Merging then! |
Pretty ugly solution but it does the trick.