search

zgrossbart / jdd

A semantic JSON compare tool
http://www.jsondiff.com
Apache License 2.0
1.05k stars 183 forks source link

Feature no underscore: removed underscore js dependency #19

Closed gangadharjannu closed 6 years ago

gangadharjannu commented 6 years ago

created two utility functions

  1. forEach for converting iterating over node list.
  2. getType for fixing typeof and getting correct type of variable.

Replaced below methods: _.isObject, _.isString, _.isNumber, _.isBoolean, _.isNull, _.isArray with custom getType method _.each with Array.prototype.forEach _.filter with Array.prototype.filter _.find with Array.prototype.find _.findIndex with Array.prototype.findIndex _.indexOf with Array.prototype.indexOf

Fixed lintintg issues code indentation according to visual studio code. created two utility functions

  1. forEach for converting iterating over node list.
  2. getType for fixing typeof and getting correct type of variable.

Replaced below methods: _.isObject, _.isString, _.isNumber, _.isBoolean, _.isNull, _.isArray with custom getType method _.each with Array.prototype.forEach _.filter with Array.prototype.filter _.find with Array.prototype.find _.findIndex with Array.prototype.findIndex _.indexOf with Array.prototype.indexOf

Fixed lintintg issues code indentation according to visual studio code. Added polyfills for Array.prototype.find and Array.prototype.findIndex

zgrossbart commented 6 years ago

Thank you for this pull request. Can you please tell me more about what the goal of this change is? Are you fixing a specific bug? Adding a new feature? Just refactoring the code?

gangadharjannu commented 6 years ago

HI @zgrossbart I have removed the dependency of underscore.js which weighs around 20Kb. Yes, I would say this as refactoring.

zgrossbart commented 6 years ago

Saving 20 kb is great, but it looks like you made a lot more changes than that. Can you please let me know more about the changes you made. Also, what browsers have you tested on? What about older version?

gangadharjannu commented 6 years ago

Before answering your question. Can you tell me the current browser support?

zgrossbart commented 6 years ago

I'm trying to support Chrome latest, Safari latest, Firefox latest, IE, and Edge. I'm also trying to make sure it works well on all mobile devices.

gangadharjannu commented 6 years ago

I've tested in all evergreen browsers. Please refer Browser_compatibility section in my first comment to see the browser compatibility. I'm supporting all evergreen browsers including IE11. In addition to that I've added the required polyfills for cross browser compatibility. So it should work across all the devices.

gangadharjannu commented 6 years ago

Regarding the changes

I already mentioned in my first comment. Most changes are related to code indentation as per airbnb style guide.

gangadharjannu commented 6 years ago

Kindly see the diffs by removing whitespace in diff settings. I've indented as per airbnb style guide. If you want I can remove the indentation changes and will just push my changes.

gangadharjannu commented 6 years ago

Mostly the changes that you suggested are from rock solid polyfills provided by Mozilla. The other change regarding jdd namespace is feasible and in fact I wanted to do so. However as I explained in my comment I defined outside just for separating concerns.