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

❓[SUPPORT]: removeItem repairType object possibly undefined #1715

Closed
developer239 opened this issue Jan 8, 2021 · 3 comments
Closed

❓[SUPPORT]: removeItem repairType object possibly undefined #1715

developer239 opened this issue Jan 8, 2021 · 3 comments
Labels
discussion An issue that discusses design decisions. good first issue Good for newcomers investigate Requires some investigation not an issue An issue logged that doesn't require fixing (maybe just a question) released
Milestone

Comments

@developer239
Copy link

developer239 commented Jan 8, 2021

When I use removeItem state operator then TypeScript complains that message object can be possibly undefined even though I specified the type for removeItem operator. I could fix the issue with optional chaining but that is not possible if I want to use a utility library like Ramda

image

With Ramda:

image

How do people use state operators? Do they really use optional chaining EVERY time they use state operator? 🙂

@developer239 developer239 added discussion An issue that discusses design decisions. good first issue Good for newcomers investigate Requires some investigation not an issue An issue logged that doesn't require fixing (maybe just a question) labels Jan 8, 2021
@developer239
Copy link
Author

developer239 commented Jan 8, 2021

After looking at the codebase I would summarize the problem like this:

When I am using removeItem I want it to do two things:

  • find a relevant item that matches my selectors
  • remove relevant item

Making the value in Predicate type optional forces me to do a third thing:

  • consider what to do with items that are not defined if there are undefined items in the array

I don't think that selector should be responsible for the third step.

// packages/store/operators/src/remove-item.ts

export function removeItem<T>(
  selector: number | Predicate<T>
): StateOperator<RepairType<T>[]> { // **existing is NEVER undefined**
  return function removeItemOperator(existing: Readonly<RepairType<T>[]>): RepairType<T>[] {
    let index = -1;

    if (isPredicate(selector)) {
      // **selector might fail here in some cases if there are undefined items in the array but we could filter the array before using the selector.** Selector should receive only items that it was designed to select from (in this case `T`)
      index = existing.findIndex(selector);
    } else if (isNumber(selector)) {
      index = selector;
    }

    if (invalidIndex(index)) {
      return existing as RepairType<T>[];
    }

    const clone = existing.slice();
    clone.splice(index, 1);
    return clone;
  };
}

I can create PR with a fix but I am not sure how would that affect the rest of the codebase but I am pretty sure that this will be too big of a breaking change to go through. I decided to create my own selectors instead.

@markwhitfeld
Copy link
Member

Great news! v3.8.0 has been released and it includes some additional types exposed for building operators.
We are closing this issue, because the improvements may assist you in resolving this issue.
Please re-open it if the issue is not resolved.

Please leave a comment in the v3.8.0 release discussion if you come across any regressions with respect to your issue.

@developer239
Copy link
Author

@markwhitfeld Thank you 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion An issue that discusses design decisions. good first issue Good for newcomers investigate Requires some investigation not an issue An issue logged that doesn't require fixing (maybe just a question) released
Projects
None yet
Development

No branches or pull requests

3 participants