Replace js-extend with Object.assign (with polyfill) #6012

Closed
andrew-aladev opened this Issue Dec 14, 2016 · 5 comments

Projects

None yet

3 participants

@andrew-aladev
Contributor

Hello. We are trying to use forever agent as an option for ajaxCore -> request.js. js-extend tries to make a deep copy of this agent. Sometimes it is possible, but sometimes it creates a recursion overflow, because existing freeSocket is a circular object.

I am sure that we should not use deep copy of request.js options. http.Agent is just an simple example.

I've replaced packages/node_modules/pouchdb-utils/src/jsExtend.js with Object.assign and npm run test-node passed.

I see failed unit tests: Test two levels merging, Test multilevel merging, Test if non object argument raises exception. I've tried to find a reason for these tests but I failed. It looks like all extend calls does not require multilevel merging and non object exception.

Can you replace js-extend with object-assign?

@nolanlawson
Member

Can you provide a failing test case so we can understand what's failing about jsExtend? And yes, we are interested in removing our polyfills, so replacing jsExtend with Object.assign isn't out of the question.

@andrew-aladev
Contributor
andrew-aladev commented Dec 16, 2016 edited
var jsExtend = require('js-extend');

var circular = {};
circular.circular = circular;

var defaults = { a: null };
var options  = { a: circular };
console.log(Object.assign({}, defaults, options));
console.log(jsExtend.extend({}, defaults, options));

defaults = { a: { circular: {} } };
console.log(Object.assign({}, defaults, options));
console.log(jsExtend.extend({}, defaults, options));

defaults = { a: circular };
console.log(Object.assign({}, defaults, options));
console.log(jsExtend.extend({}, defaults, options));

{ a: { circular: [Circular] } }
{ a: { circular: [Circular] } }
{ a: { circular: [Circular] } }
{ a: { circular: { circular: [Object] } } }
{ a: { circular: [Circular] } }
Maximum call stack size exceeded

Please look here.

var reqOpts = extend(clone(ajaxOpts), userOpts.ajax, options);

We are passing our custom agent as an ajax option. But ajaxOpts already has another agent. If both agents are circular - it will create a hang and resursion overflow. This exception appears randomly because both http agents can be circular or not circular depends on it's usage. But pouchdb shouldn't ever try to merge these objects.

Please look at these options ajaxOpts, userOpts.ajax once again. They don't want to be merged with deep copy. They wants just Object.assign. I can see that all pouchdb project don't need a deep copy. I think all extend calls can be replaced with Object.assign.

@nolanlawson nolanlawson changed the title from Redundant js-extend to Replace js-extend with Object.assign (with polyfill) Dec 16, 2016
@nolanlawson
Member

Thanks, I agree we can probably replace jsExtend with an Object.assign+polyfill. I think we are only ever using it for the shallow copy, not the deep copy.

@andrew-aladev
Contributor

Thank you. It works good.

@willholley
Member

It turns out we need deep copy/merge for the request headers, but perhaps we can treat this as a special case.

@willholley willholley added a commit that referenced this issue Jan 20, 2017
@willholley willholley (#6132) - merge user-specified and default headers
b325846 changed the behaviour of the HTTP adapter so that a
headers object supplied in requets options would overwrite the
default request headers. This results in authentication headers
not being added for certain PouchDB operations (e.g. put attachents)
and breaks instances where users provide custom headers.

Rather than re-introducing deep object extension across the library,
treat the headers object as a special case and explicitly merge default
and request-specific headers when construction the request options.

Fixes #6132, refs #6012
c268149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment