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(Entity): Do not add Array.prototype properties to store #782

Merged
merged 1 commit into from Feb 9, 2018

Conversation

Projects
None yet
3 participants
@livthomas
Copy link
Contributor

livthomas commented Feb 4, 2018

I have tried to fix issue #781 and verified the problem is really no longer there by building my application (where I can always reproduce the problem) with a version of @ngrx/entity containing these changes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 4, 2018

Coverage Status

Coverage increased (+0.2%) to 93.016% when pulling f311f39 on livthomas:entity-null into ecf1ebf on ngrx:master.

@MikeRyanDev

This comment has been minimized.

Copy link
Member

MikeRyanDev commented Feb 5, 2018

It sounds like this is hard to reproduce. Any chance you can isolate it into a unit test that fails on current version but passes with this fix?

@livthomas

This comment has been minimized.

Copy link
Contributor Author

livthomas commented Feb 6, 2018

I simply cannot reproduce it in the unit tests. But I think I have found the root cause. My project uses core-js library which adds some additional Array.prototype methods and the for..in loop in @ngrx/entity code then iterates over them (as well as over the indices).

In general, iterating over an array with for..in loop is a very bad idea and should be avoided. See the relevant sections of Speaking JavaScript book or MDN documentation.

@livthomas livthomas force-pushed the livthomas:entity-null branch from f30509c to f311f39 Feb 9, 2018

@livthomas livthomas changed the title fix(Entity): Do not store null entity keys fix(Entity): Do not add Array.prototype properties to store Feb 9, 2018

@livthomas

This comment has been minimized.

Copy link
Contributor Author

livthomas commented Feb 9, 2018

@MikeRyanDev, I have added a reproducer here as you suggested.

@MikeRyanDev MikeRyanDev merged commit d537758 into ngrx:master Feb 9, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
coverage/coveralls Coverage increased (+0.2%) to 93.016%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment