fix: support Objects created with Object.create({}) #842
Conversation
googlebot
added
the
cla: yes
schmidt-sebastian
force-pushed
the
mrschmidt/plainobject
branch
from
960a670
to
10d5d0c
Compare
Codecov Report
@@ Coverage Diff @@
## master #842 +/- ##
==========================================
+ Coverage 88.82% 95.36% +6.54%
==========================================
Files 27 25 -2
Lines 16755 15138 -1617
Branches 1156 1151 -5
==========================================
- Hits 14882 14437 -445
+ Misses 1868 695 -1173
- Partials 5 6 +1
Continue to review full report at Codecov.
|
| isObject(input) && | ||
| (Object.getPrototypeOf(input) === Object.prototype || | ||
| Object.getPrototypeOf(input) === null || | ||
| input.constructor.name === 'Object') |
There was a problem hiding this comment.
According to this stackoverflow post, obj.constructor can be changed by the user, so they recommend using instanceof instead.
There was a problem hiding this comment.
Unfortunately, the recommendation here does not quite work for us. We don't have access to the user's type and instanceof Object returns true for custom types as well as plain objects such as {}.
It also looks like the other suggestion Object.prototype.toString.call just returns [object Object] for custom types, so that is likely out of the question too.
dev/test/index.ts
Outdated
| @@ -1073,7 +1073,7 @@ describe('getAll() method', () => { | |||
| expect(actualErrorAttempts).to.deep.eq(expectedErrorAttempts); | |||
| }); | |||
| }); | |||
| }); | |||
| }).timeout(5000); | |||
There was a problem hiding this comment.
This is in master now, so the diff should eventually disappear: https://github.com/googleapis/nodejs-firestore/blob/master/dev/test/index.ts#L1103
There was a problem hiding this comment.
As discussed in person, consider adding unit tests to match the is-plain-object results from the npm package.
dev/test/util.ts
Outdated
| import {isPlainObject} from '../src/util'; | ||
|
|
||
| describe('util', () => { | ||
| it('isPlainObject supports Object.create()', () => { |
There was a problem hiding this comment.
nitting for nits sake: Replace 'supports' with 'allows'?
schmidt-sebastian
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
This changes our plain object check to use the constructor name, and also uses the constructor name in all of our object checks throughout.
fixes: #799