Giant refactor #14
Giant refactor #14
Conversation
object-is has semantically identical code (it is a polyfill for Object.is which uses ES6 sameValue). By removing our implementation of sameValue we remove any extra burden on maintaining that code
|
How is this coming? |
|
This is pretty much at final draft stage, I'd like to get people's thoughts (@meeber, @lucasfcosta, @hildjj) for any serious roadblockers, I have one or two tests to fix, then I'm going to clean up the readme and commits and people can take a final review before we get it merged. |
|
Has it been confirmed that #11 is fixed in this branch? |
|
Hi. I saw this module, and I can supply with this:
new Set([
[],
[],
]);or even deeper then this
// simple === comparison on primitives
lodash x 55,261,863 ops/sec ±1.11% (91 runs sampled)
nodejS assert x 45,242,272 ops/sec ±1.09% (87 runs sampled)
chai x 16,690,551 ops/sec ±0.41% (92 runs sampled) |
|
@keithamus @meeber The last days I started to work on a similiar algorithm, and I found more issues in your current implementation. When run in browsers, you will get trouble with the core.js library with your feature detection. Example Symbol(). There are also issues with your generator implementation, and some other things. I would recommend to run a benchmark. Update |
|
@keithamus I updated the benchmark results on NPM for kewlr. Look at the readme. // chai
string literal x 4,255,021 ops/sec ±1.08% (88 runs sampled
// kewlr
string literal x 72,032,383 ops/sec ±1.56% (87 runs sampled)
// lodash
string literal x 56,845,867 ops/sec ±2.48% (85 runs sampled) |
This commit removes the bootstrap, which is used for testing in the browser, switching instead to Karma, using browserify - which means `require` works in both the browser and node, negating the need for the bootstrap and index.html
Only include `index.js` and `type-detect.js` (npm will automatically include `package.json` and `README.md` for us
Add support for ES6 types BREAKING CHANGE: Many types will now return `false` if they are not deeply equal. Types which are not recognised will likely default to returning `false`. Beforehand, unrecognised types would return `true`
keithamus
force-pushed
the
giant-refactor
branch
from
ef7fc7d
to
152d8a6
Compare
|
Just pushed up some changes, which, IMO completes this PR. Circular references are fixed. I've measured bench speed to be at least on par if not faster than the similar libs out there. @zubuzon want to give this one more run through? Then if @meeber or @lucasfcosta could give it a review, that'd be grand 😄 |
|
@keithamus I have a client meeting now, but will reply within 6 - 7 hours. I noticed you are using the typeof operator for string and booleans. That's not safe cross-browser. Or it depends. Safari 5 - 9 have major issues, and Safari 10 when it comes to function detection and |
|
Thanks for that @zubuzon. I'm about to push up some final changes which address some performance concerns, and it looks like it puts kewlr and deep-eql neck and neck with each other. Plus your comments here helped me find some perf issues around type-detect - so thanks for your awesome work! @meeber @lucasfcosta I'd say when I push up these changes this will be a 100% complete PR. I don't want to make any more changes to it (unless of course anything comes up in your final review). But I'd say we're good to go, and good to fold into chai 4.0! |
|
@keithamus that looks awesome! I'll be happy to review your changes as soon as you push them 😄 |
BREAKING CHANGE: The third argument given to deep-eql is now an options object, which takes the properties `comparator` and/or `memoize`.
keithamus
force-pushed
the
giant-refactor
branch
from
152d8a6
to
0cb370a
Compare
|
@lucasfcosta @meeber pushed! Let's merge this thing! |
|
FYI we should probably wait for chaijs/type-detect#79 before merging... in fact all the tests are failing because it has been written against that version. |
|
@keithamus Glad i could help :) Chai is a awsome library!! A last tip. I saw you was still using forEach for Map() and Set() too solve IE11 issue. In Kewlr I feature detect IE, and only using forEach for IE, and faster solution for everything else! |
|
LGTM! Excellent work @keithamus and @zubuzon! Edit: Assuming type-detect gets updated to 3.0.0 in package.json and the tests pass of course :D |
|
@keithamus @meeber @lucasfcosta I wish you all three luck with Chai. "Chai" is sort of a word everyone knows, and that is something to be proud of! This was the first time I contributed in open source, and I'm now off Github and back to RL and my companies. I will continue ofc to use Chai as everyone else! A tip for next release - 4.0 - would be not only offer a common.js module, but also NodeJS module. Many libraries now offer this, and it's the future to come. And make it easier for end-users to import this library! My time is up, and once again! Great work with this! And good luck to you all! ZubuZon |
keithamus
force-pushed
the
giant-refactor
branch
from
0cb370a
to
1d2f040
Compare
|
@lucasfcosta can we get your seal of approval and merge this sucker? |
|
LGTM too! It's great to work with such talented people! |
|
|
||
| var comparatorResult = comparator && comparator(leftHandOperand, rightHandOperand); | ||
| if (comparatorResult === false || comparatorResult === true) { | ||
| memoizeSet(leftHandOperand, rightHandOperand, options.memoize, comparatorResult); |
There was a problem hiding this comment.
Why set these twice, rather than check both in the memoizeResult check on L161?
We've had a few PRs against deep-eql which overlap some of this work. I'm just going to set up this PR for now, as a placeholder so we can refer to it while I finish the work left on it.
This will fix #13, #7, #4 and possibly #9
Also supersedes #12