search

tshaddix / webext-redux

A set of utilities for building Redux applications in Web Extensions.
MIT License
1.22k stars 180 forks source link

Basic array diff #202

Closed srvance closed 5 years ago

srvance commented 5 years ago

This PR adds basic array diffing based on === using the fast-array-diff module. We could pretty easily change it to do a deep equality check, but it would take a lot more to recurse into those diffs with object diffs.

This was motivated by the performance challenges trying to use redux-undo with webext-redux. redux-undo creates past and future arrays, so most of the changes are a single add or remove on one end or the other and since they preserve object identity, === is sufficient.

tshaddix commented 5 years ago

Hi @srvance - thank you for this contribution!

I'm wondering if we might be able to pull the methods out of fast-array-diff that we are using and include them in this package. We can put a reference to make sure credit is given where credit is due to the author of fast-array-diff. What do you think?

Recent NPM exploits have me on edge a bit.

srvance commented 5 years ago

I think we can do that. Extending the array diff deeper would pretty much require that anyway. Eventually, it should recognize moves, as well.

srvance commented 5 years ago

They’re natively in Typescript. Would you want them that way or ported to JS?

tshaddix commented 5 years ago

@srvance Ideally we just port to JS for now. I have a desire to move this entire package to Typescript eventually but that will take some time.

srvance commented 5 years ago

Let me take a look at what it would take to port it. Would you be willing to have this project hybrid TS/JS if that's an easier path? We could then migrate the rest at our leisure.

srvance commented 5 years ago

@tshaddix Copied the fast-array-diff files in and ported the TS to JS, including full test coverage. Felt a little dirty removing all the type information, but I agree that it's best for now.

tshaddix commented 5 years ago

@srvance This is excellent work. Thank you for doing this as well as documenting it and including tests. I'm going to merge this in to master and do some smoke tests before bumping versions!

srvance commented 5 years ago

Thanks, @tshaddix. Glad to contribute. Looking forward to the release that includes it.

srvance commented 5 years ago

@tshaddix Any idea when you’ll cut a release with this in it?

tshaddix commented 5 years ago

I'll make it happen this weekend!

tshaddix commented 5 years ago

Just kidding - it's out now.

srvance commented 5 years ago

Awesome! Thank you!