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 a bug in ObjectUtils::areSimpleEquivalent method #1979

Merged

Conversation

jeremco
Copy link
Contributor

@jeremco jeremco commented Jun 1, 2017

If objects have same parameters but declared in a different order, string obtained using JSON.stringify() will be different and the method will return false, whereas it should return true.

The fix uses deep-equal module to avoid this bug.

I have although added a unit test to test the solution (in streaming.utils.ObjectUtils.js file) : using JSON.stringify(), the unit test fails, using deep-equal module, the test is in success

If objects phave same parameters but declared in a different order, string obtained using JSON.stringify will be different, the method will return false, whereas it should return true.

The fix uses deep-equal module to avoid this bug.
Add a unit test to test the solution : using JSON.stringify, the unit test fails, using deep-equal module, the test is in success
@davemevans
Copy link
Contributor

This isn't a bug - the whole point of this method was to avoid importing a module and simply compare objects constructed in a predictable manner - see the jsdoc above. With this PR the method has no value now so the whole objectutils could be removed and deep-equal imported where needed.

@dsparacio
Copy link
Contributor

Guys is this needed or not. If not please close or please merge.

@jeremco
Copy link
Contributor Author

jeremco commented Jun 6, 2017

Hi guys

From my point of view, if you are planning to release an industrial product, I think we should correct this potential bug (even if the code is predictable).
Moreover, I think keeping objectUtils has a meaning : for one ore more reasons (performances of used module, bug in module ...), it is more easy to replace one external module in one file, rather than in several files. Doing this, you decrease the risk of regressions.

It's up to you to merge or not.

@davemevans
Copy link
Contributor

I still think it'd be better to change the method name, or have an additional method for deep compare, (areEquivalent?), but I don't feel that strongly.

@jeremco
Copy link
Contributor Author

jeremco commented Jun 6, 2017

I have renamed ObjectUtils::areSimpleEquivalent as ObjectUtils::areEqual to avoid any confusion

Copy link
Contributor

@davemevans davemevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@davemevans davemevans merged commit a2521b2 into Dash-Industry-Forum:development Jun 6, 2017
@jeremco jeremco deleted the ObjectUtils_Bug branch June 6, 2017 14:56
jeremco pushed a commit to Orange-OpenSource/dash.js that referenced this pull request Jun 12, 2017
davemevans pushed a commit that referenced this pull request Jun 13, 2017
…tTest

Fix bad merge when PR #1979 and PR #1956 have been merged in development
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.

3 participants