Skip to content
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 merging objects with prototypes #201

Merged
merged 4 commits into from
Apr 19, 2020

Conversation

aleksandryackovlev
Copy link
Contributor

The merge function in /source/merge.js uses Object.assign method.
The issue is that this method copies only an object's own properties to the resulting object.

It means we've got an error from https://github.com/nodejs/node/blob/master/lib/_http_client.js#L135 when we pass httpsAgent or httpAgent as an option to any method of the client.

For example, the following code will throw th an error.

await client.putFileContents(filePath, fileBuffer, { httpsAgent: new https.Agent({ rejectUnauthorized: true })});

To fix this issue we have to not only copy its own properties of the original object but to copy its prototype too.

source/merge.js Outdated Show resolved Hide resolved
@perry-mitchell
Copy link
Owner

Hmm.. very interesting. I made a suggestion, to improve the cloning process, but I simply wonder if a helper like the following would be better:

function isPureObject(input) {
    if (input === null || typeof input === "undefined") return false;
    return Object.getPrototypeOf(input).isPrototypeOf(Object);
}

We could use this to not merge objects that are non-object types (classes, like HttpsAgent).

We'd need to also test for Array.isArray() as [] doesn't satisfy isPureObject of course.

@perry-mitchell perry-mitchell self-assigned this Mar 31, 2020
@aleksandryackovlev
Copy link
Contributor Author

Maybe it's going to be better to combine both approaches.
It seems like in lodash.merge they use both of them.
https://github.com/lodash/lodash/blob/master/.internal/baseMergeDeep.js

@perry-mitchell
Copy link
Owner

Thanks @aleksandryackovlev! This is great. Will merge shortly.

@perry-mitchell perry-mitchell merged commit 4180ecb into perry-mitchell:master Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants