`prefer-spread` flagging `Array#concat(…)` for non-iterable objects

[bdunks]

After upgrading to 27.0.0, I'm finding that the prefer-spread rule is raising ERROR_ARRAY_CONCAT errors when a concat method exists on non-iterable objects.

Specifically, this I've witnessed this occurring when chaining an ApolloLink for the ApolloClient. The ApolloLink object defines a static concat method.

const client = new ApolloClient({
  link: authLink.concat(httpLink),  //<-- Raises `Array#concat(…)` error
  cache: new InMemoryCache({}),
});

In this case, authLink is of type ApolloLink, which has the following type definition:

export declare class ApolloLink {
    static empty(): ApolloLink;
    static from(links: (ApolloLink | RequestHandler)[]): ApolloLink;
    static split(test: (op: Operation) => boolean, left: ApolloLink | RequestHandler, right?: ApolloLink | RequestHandler): ApolloLink;
    static execute(link: ApolloLink, operation: GraphQLRequest): Observable<FetchResult>;
    static concat(first: ApolloLink | RequestHandler, second: ApolloLink | RequestHandler): ApolloLink;
    constructor(request?: RequestHandler);
    split(test: (op: Operation) => boolean, left: ApolloLink | RequestHandler, right?: ApolloLink | RequestHandler): ApolloLink;
    concat(next: ApolloLink | RequestHandler): ApolloLink;
    request(operation: Operation, forward?: NextLink): Observable<FetchResult> | null;
    protected onError(error: any, observer?: ZenObservable.Observer<FetchResult>): false | void;
    setOnError(fn: ApolloLink["onError"]): this;
}

[fisker]

Sorry, we can't know the type of authLink

[sindresorhus]

The only way we can improve this is with https://github.com/sindresorhus/eslint-plugin-unicorn/issues/347.

[bdunks]

Hi @fisker - I'm sorry, I should have posted an example that doesn't rely on a third party library. Hopefully it's okay that this is illustrated as a react component; I believe it gets the point across.

class Chainable {
  concat(newLink: Chainable){
    console.log("I don't do what you expect");
  }
}

export const Component = () => {
  const notAnArray = new Chainable();
  const alsoNotAnArray = new Chainable();
  const chainedItem = notAnArray.concat(alsoNotAnArray); //<-- Raises `Array#concat(…)` error
  return (
    <></>
  );
}

I've found that adding object: 'Array', to the rule definition's method selector will prevent the issue (similar to the arrayFromCallSelector just above), but I don't know the broader implications of this change.

[fisker]

Adding object: 'Array' means the selector only matches Array.concat() it's for static methods.

[bdunks]

Got it, thank you.

Do you agree it doesn't make sense to error on arbitrary objects that cannot be spread?

If we can't validate that the object is of type array, could we consider adding explicit ignore options similar the catch-error-name rule, checking the node.callee.object.name against the ignore list?

This might allow a rule definition like this, which would be helpful when we want the rule enabled in general, but have isolated cases that a third party library defines classes with a concat method:

            'unicorn/prefer-spread': [
                'error',
                {
                    ignore: [/^authLink/i],
                },
            ],

[JounQin]

Or can we add an option prefer concat?

// better to me, specially for ts files
const singleOrArr = xx

const arr2 = arr1.concat(item, arr2, singleOrArr)

const arr3 = [...arr1, item, ...arr2, ...(Array.isArray(singleOrArr) ? singleOrArr : [singleOrArr])]

[fisker]

We may consider and option to ignore .concat(), but absolutely not suggesting the opposite.

[fisker]

FYI: I just finished refactoring Prettier codebase from .concat() to ... , it's much more clear that the argument is an array or not, and during that refactor, I realized Array.isArray test is useful suggestion, and already added to the rule #1054

[JounQin]

We may consider and option to ignore .concat(), but absolutely not suggesting the opposite.

Yeah, I mean an option to ignore concat, I think it's useful personally.

[bdunks]

I've spent a bit more time digging in, and I'm wondering if this conditional/check should be changed to:

if (!staticResult || !Array.isArray(staticResult.value)) {
    return;
}

The rule currently bails out if it a staticResult is not an Array. However, it doesn't bail if there is no staticResult. In the case of an arbitrary object, staticResult is null.

This would also address #1068 without relying on the callee.object.name starting with a capital letter.

Note that this causes a number of existing tests to fail (e.g., 'foo.concat(2)', is no longer an invalid case), however, perhaps those test should also more explicitly ensure we are testing arrays. The following case is still invalid:

outdent`
    const foo = [];
    foo.concat(2);
`,

[fisker]

I'm wondering if this conditional/check should be changed to:

No, we can't, most time staticResult can't be calculated, even this simple code

class A {};

const foo = [1, 2, new A()];

The staticResult is null

---

Of cause, this value is unknown too.

import foo from './foo';

const bar = foo.concat(1)

I added that code, just to prevent String#concat(),

const foo = '1';
const bar = foo.concat("2");

[moltar]

This happens with Buffer too, which is a built in.

[Nantris]

Just hit this myself. Seems like a shame to need to disable prefer-spread entirely to avoid this. It would be great it the rule excluded concat.

[the-ult]

hmm. Could this be an issue with a method that returns a string as well?

while this isn't a problem:

test(){
   const myTestString = 'hello';
    const result = myTestString.concat("this is a test");
  }