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

Extract deepEqual implementation with tests to share it with other libraries? #865

Open
jzaefferer opened this Issue Sep 29, 2015 · 15 comments

Comments

4 participants
@jzaefferer
Member

jzaefferer commented Sep 29, 2015

At least node has its own implementation of deepEqual, as part of the assert module. I have no idea how close their implementation is to ours, might be worth investigating.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 16, 2015

Member

Looking around a bit, I found three distinct implementations, all with their own tests. There's definitely potential to share at least tests, better yet implementation.

Not sure how to go about this, and don't yet know the history behind any of these.

Member

jzaefferer commented Oct 16, 2015

Looking around a bit, I found three distinct implementations, all with their own tests. There's definitely potential to share at least tests, better yet implementation.

Not sure how to go about this, and don't yet know the history behind any of these.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 16, 2015

Member

For anyone interested in this: Should try to port QUnit's tests to one or all of the other modules and see what fails. We can use those failures to start a discussion with the module owner, along with talking about browser support and whatever else.

Member

jzaefferer commented Oct 16, 2015

For anyone interested in this: Should try to port QUnit's tests to one or all of the other modules and see what fails. We can use those failures to start a discussion with the module owner, along with talking about browser support and whatever else.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 17, 2015

Member

After talking to @jdalton about this: We should try to replace our deepEqual implementation with the one in lodash. We can run our existing tests to see if there are (significant) differences in the design. If it works out well, we could promote the lodash implementation to those other projects listed above.

Member

jzaefferer commented Oct 17, 2015

After talking to @jdalton about this: We should try to replace our deepEqual implementation with the one in lodash. We can run our existing tests to see if there are (significant) differences in the design. If it works out well, we could promote the lodash implementation to those other projects listed above.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 17, 2015

Member

@jdalton I've read https://lodash.com/custom-builds and some other resources, but don't see an option for a (custom) compat build with just isEqual (with dependencies, global or umd exports). What am I missing?

Member

jzaefferer commented Oct 17, 2015

@jdalton I've read https://lodash.com/custom-builds and some other resources, but don't see an option for a (custom) compat build with just isEqual (with dependencies, global or umd exports). What am I missing?

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Oct 17, 2015

via lodash-cli:

lodash include=isEqual

of via browserify after you npm i lodash.isequal:

browserify -r lodash.isequal -s isEqual -o lodash.isequal.js

jdalton commented Oct 17, 2015

via lodash-cli:

lodash include=isEqual

of via browserify after you npm i lodash.isequal:

browserify -r lodash.isequal -s isEqual -o lodash.isequal.js
@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 17, 2015

Member

I just tried to replace the QUnit.equiv call in assert.js to use _.isEqual instead, and all tests still pass. That's great!

Still need to figure out how to properly integrate lodash into our build, since this is the first external dependency.

Member

jzaefferer commented Oct 17, 2015

I just tried to replace the QUnit.equiv call in assert.js to use _.isEqual instead, and all tests still pass. That's great!

Still need to figure out how to properly integrate lodash into our build, since this is the first external dependency.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Oct 21, 2015

Still need to figure out how to properly integrate lodash into our build, since this is the first external dependency.

You could go the webpack route and include isEqual as either require('lodash/lang/isEqual') or require('lodash.isequal').

I can prep a PR tonight to flesh a possible approach out.

jdalton commented Oct 21, 2015

Still need to figure out how to properly integrate lodash into our build, since this is the first external dependency.

You could go the webpack route and include isEqual as either require('lodash/lang/isEqual') or require('lodash.isequal').

I can prep a PR tonight to flesh a possible approach out.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 21, 2015

Member

That would be great, @jdalton!

It looks like replacing internal deepEqual is the easiest part, but it needs some good strategy using webpack/browserify.

Member

leobalter commented Oct 21, 2015

That would be great, @jdalton!

It looks like replacing internal deepEqual is the easiest part, but it needs some good strategy using webpack/browserify.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 21, 2015

Member

Yeah, switching to internal modules internally is currently the bigger issue. Though if we make that work with webpack, it'll be super easy to add external modules.

Member

jzaefferer commented Oct 21, 2015

Yeah, switching to internal modules internally is currently the bigger issue. Though if we make that work with webpack, it'll be super easy to add external modules.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 29, 2015

Member

@jdalton any idea if you'll be able to work on this?

Member

jzaefferer commented Oct 29, 2015

@jdalton any idea if you'll be able to work on this?

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Oct 29, 2015

Yes, I'll try to get something working this weekend. Last weekend I was unblocking rollup (es6 module bundler). This weekend can be QUnit bundling time. The switch may want to be put on hold though until after the 4.0 bump as lodash's 3.10.1 doesn't support map/set comparisons.

jdalton commented Oct 29, 2015

Yes, I'll try to get something working this weekend. Last weekend I was unblocking rollup (es6 module bundler). This weekend can be QUnit bundling time. The switch may want to be put on hold though until after the 4.0 bump as lodash's 3.10.1 doesn't support map/set comparisons.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 29, 2015

Member

Sounds goood. Making progress on bundling will be valuable either way.

Member

jzaefferer commented Oct 29, 2015

Sounds goood. Making progress on bundling will be valuable either way.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Feb 9, 2016

I should revisit this now that v4 is out.

jdalton commented Feb 9, 2016

I should revisit this now that v4 is out.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Feb 9, 2016

Member

That would be great! I think our deepEqual implementation has seen some improvements, but that shouldn't stop us from replacing it.

Member

jzaefferer commented Feb 9, 2016

That would be great! I think our deepEqual implementation has seen some improvements, but that shouldn't stop us from replacing it.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Feb 16, 2016

Member

I think our deepEqual implementation has seen some improvements, but that shouldn't stop us from replacing it.

yes, I'm looking forward to it.

Member

leobalter commented Feb 16, 2016

I think our deepEqual implementation has seen some improvements, but that shouldn't stop us from replacing it.

yes, I'm looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment