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

Making containsUndefined check for own properties #1711

Merged
merged 1 commit into from Oct 31, 2017
Merged

Conversation

@shimeez
Copy link
Contributor

@shimeez shimeez commented Sep 30, 2016

No description provided.

@shimeez shimeez force-pushed the shimeez:master branch from f087624 to d150d61 Nov 9, 2016
@elhigu
Copy link
Member

@elhigu elhigu commented Nov 29, 2016

Hi, what is the motivation for this pull request? Also tests are required also for small changes like this.

@dj-foxxy
Copy link

@dj-foxxy dj-foxxy commented Dec 5, 2016

I've got a max call stack exceeded error that this pull request might be addressing. The error:

... RangeError: Maximum call stack size exceeded
at isFunction (/home/.../meteor/node_modules/lodash/isFunction.js:27:20)
at Object.containsUndefined (/home/.../node_modules/knex/lib/helpers.js:102:41)
...

I'm using knex@0.15.6 and lodash@4.17.2 as part of a Meteor application. The error occurs on the Node.js v4.6.2 server (not tested on client).

@dj-foxxy
Copy link

@dj-foxxy dj-foxxy commented Dec 5, 2016

I just hacked this commit into our code base and it stops it exploding. I have no idea of the wider impact but I'm sticking it in there 'cos I aint getting on where with out it. :)

@shimeez shimeez force-pushed the shimeez:master branch from d150d61 to c381cf2 Dec 20, 2016
@elhigu
Copy link
Member

@elhigu elhigu commented Oct 31, 2017

I read through code where this is used. Seems legit and useful and shouldn't break anything.

@elhigu elhigu merged commit 53314cb into knex:master Oct 31, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants