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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always use Array.isArray to check if array #521
Conversation
Pull Request Test Coverage Report for Build 817
馃挍 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 817
馃挍 - Coveralls |
Pull Request Test Coverage Report for Build 817
馃挍 - Coveralls |
Pull Request Test Coverage Report for Build 833
馃挍 - Coveralls |
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.
My team found this to be an issue when trying to use the Model.batchPut.
Mind adding a test case for this that fails without this PR and is successful with it? That would also be good because it would ensure your project doesn't break in the future.
Just pushed a test, please take a look @fishcharlie. |
@dobrynin Looks like there is a merge conflict. Wanna try to solve that? |
@dobrynin And just to confirm. That test you added, fails without your changes, correct? |
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.
LGTM
All set. The test fails without these changes. The test is for the specific case that gave us trouble (batchPut) |
Summary:
Change usage of
array instanceOf Array
andutil.isArray(array)
toArray.isArray(array)
.My team found this to be an issue when trying to use the
Model.batchPut
.Other information:
array instanceof Array
breaks when multipleArray
globals exist: http://web.mit.edu/jwalden/www/isArray.htmlutil.isArray
is deprecated: https://nodejs.org/api/util.html#util_util_isarray_objectType (select 1):
Is this a breaking change? (select 1):
Is this ready to be merged into Dynamoose? (select 1):
Are all the tests currently passing on this PR? (select 1):
Other:
npm test
from the root of the project directory to ensure all tests continue to pass