Skip to content
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

Improve array patches #276

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

orengriffin
Copy link
Contributor

we at Optibus have an issue with patches that are created when two arrays are compared - the second is a filter result of the first.
instead of having a simple remove patch, we can get a few hundred (or even thousands) of replace patches.

the suggested code, using lodash's 'difference', is trying to detect those cases and use a simple 'remove' or 'add' patch.
it also reverts to old code in cases where:
a. the result is simply wrong - mostly like the difference between the arrays is a big one.
b. the differences are so great there is probably no added value.

I added 1000 tests with random arrays. so i believe the solution is solid.
regarding the tests that fail - i would consider removing them

@Starcounter-Jack
Copy link
Owner

a. the result is simply wrong - mostly like the difference between the arrays is a big one.
This sounds alarming. Wrong as in producing a big set of operations (not capturing the intention of the patch or not optimizing for operation set size), or wrong as in producing a patch that does not render the correct array? Any algorithm must be sound and correct.

b. the differences are so great there is probably no added value.

What is the heuristics of evaluating when the lodash algorithm is appropriate?

@t-knapp
Copy link

t-knapp commented Feb 21, 2022

I face exactly the same problem when I compare two arrays.

@Starcounter-Jack
Copy link
Owner

This would be a major change. If you can provide a simple plugin system such that existing users are not impacted for size and performance we could accept this as a standard feature.

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.

None yet

3 participants