Skip to content

Conversation

@knalbandianbrightgrove
Copy link
Contributor

  • fix IE8 issue related from lodash.clonedeep as they don't support IE8 anymore

@CacheControl
Copy link
Owner

Thanks for the contribution @knalbandianbrightgrove

Quick explanation on the value this feature adds: The reason deep cloning is used in this part of the code is to ensure that any time the engine runs and a RuleResult object is provided, the developer can do anything they want with those results, without fear of affecting the internals of the engine. Meaning they can mutate/mutilate the ruleResult to their heart's content, and subsequent runs of the engine will work just fine.

I mention this because I notice in the docs for the clone-deep library, there's a warning about cloning object instances (this jumped out at me because json-rules-engine makes heavy use of class instances). I was a little surprised the test suite passed because I thought I'd added coverage around the use of deep cloning; turned out I'd forgotten to write that test.

I went ahead and added the missing coverage to master. After merging this into the PR above, you'll notice the test will fail; this is because clone-deep requires a callback which specifies which properties to copy from the target object instance.

I did come across an alternative package called clone which may do the trick; it does not explicitly claim support for ie8, but it does have monthly downloads in the tens of millions and is the dependency of several front end projects. Installing it and using in place of clone-deep allows the tests to pass, but I'm not in a position to test IE8 - perhaps you could give it a try?

@knalbandianbrightgrove
Copy link
Contributor Author

@CacheControl

I very appreciated, thank you for the informative explanation!!

I've checked clone in IE8, it works, I've made another PR.

Kind regards!

@CacheControl
Copy link
Owner

@knalbandianbrightgrove great, I'll go over this one more time tonight, and merge + version if everything looks good!

@CacheControl CacheControl merged commit 80cd15c into CacheControl:master Jul 25, 2017
@CacheControl
Copy link
Owner

@knalbandianbrightgrove this is published as 2.0.2, thanks again for the contribution!

@knalbandianbrightgrove
Copy link
Contributor Author

@CacheControl thank you for the awesome solution with great solution!

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.

2 participants