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

Regression: Unable to override compare after es6 refactoring #155

Closed
homj opened this issue Jun 3, 2019 · 2 comments · May be fixed by js-entity-repos/memory#302
Closed

Regression: Unable to override compare after es6 refactoring #155

homj opened this issue Jun 3, 2019 · 2 comments · May be fixed by js-entity-repos/memory#302

Comments

@homj
Copy link

homj commented Jun 3, 2019

Since sift has been refactored to es6 & cleaned up (with 8b1f4b9), the previous feature of providing a custom compare function (added via #97) is broken.

Previously, the compare function was a property of sift, but has since been declared as an exported function which can't be overridden.

I'd suggest to change this back to the previous behavior, or allow to override the compare function by passing it via options. Additionally, I'd like to override the comparable function as well (e.g. do convert a date string to a Date instance), but this is independent from the issue described above and should be tracked in another issue.

@crcn
Copy link
Owner

crcn commented Jun 11, 2019

Should be fixed in 8.4.0. You now have the option to pass a custom compare function as an option. For example:

class Item {
  constructor(value) {
    this.value = value;
  }
  compare(other) {
    return other && this.value === other.value ? 0 : -1;
  }
}

const filter = sift(new Item("a"), {
  compare(a, b) {
    if (a instanceof Item) {
      return a.compare(b);  
    }
    return compare(a, b);  
  }
});

const items = [new Item("a"), new Item("b"), new Item("a")];
const results = items.filter(filter); 

☝️ let me know if this fixes the issue for you.

I'd like to override the comparable function as well (e.g. do convert a date string to a Date instance), but this is independent from the issue described above and should be tracked in another issue.

Just added a ticket for this here: #156

@crcn crcn added the fixed label Jun 19, 2019
@crcn crcn closed this as completed Jun 19, 2019
@crcn
Copy link
Owner

crcn commented Jun 21, 2019

FYI #156 is fixed (adding custom comparable). Version for that is 8.5.0.

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