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 no-array-mutation: allow slice().sort(...) #88

Closed
chasecaleb opened this issue Aug 22, 2018 · 9 comments
Closed

Improve no-array-mutation: allow slice().sort(...) #88

chasecaleb opened this issue Aug 22, 2018 · 9 comments

Comments

@chasecaleb
Copy link

Is it possible to improve no-array-mutation (added in #84) to allow code like this?

const sorted = ["foo", "bar"].slice().sort((a, b) => a.localeCompare(b));

Since slice() creates a copy this is effectively immutable, but it still triggers the lint rule. As a result, my code is littered with // tslint:disable-next-line:no-array-mutation, which isn't ideal.

(I'm assuming that there isn't a better way to sort an array that I'm not aware of.)

@jonaskello
Copy link
Owner

In this case we would have to detect that the intention was not to mutate even though an mutative method was called (.sort()) which might be a bit awkward. Still it would be possible to add a check for this specific case (slice directly followed by sort) and make an option to allow it which might be worthwhile if it is a common pattern. Another option is of course to make a centralized function to do the sorting, then you would only have to override the linting in one place. But having to import utility functions everywhere is not ideal either. Perhaps @RebeccaStevens has some input on this?

@chasecaleb
Copy link
Author

Agreed about using utility functions being undesirable. Beyond the import concern, they're a big readability impact because you have to pass the array as a param instead of chaining it, which is ugly if e.g. there are already several preceding map()/filter()/etc calls being chained.

Adding an option for this behavior seems reasonable to make everyone happy. I can probably find time to make a PR for this if/when you're okay with the idea, but I'm just as happy if someone else does it.

@RebeccaStevens
Copy link
Collaborator

I would say we would want to go for a more generic approach than simply allowing slice().sort(...) as an exception. The way I'd suggest is to make it so that when linting a chain of method calls, if one of those calls returns a new array, array mutating methods are then allowed from that point on (in the chain).

I can look into making a PR for this tonight sometime.

@jonaskello
Copy link
Owner

@RebeccaStevens That sounds like a nice approach! Do you still think it should be an option or should the rule always work like this?

@RebeccaStevens
Copy link
Collaborator

Hmmmmm, I'm not sure actually.

@RebeccaStevens
Copy link
Collaborator

I've created a PR for this. I went with adding this as an option called ignore-mutation-following-accessor.

Let me know if you can think of a better name for this option.

@chasecaleb
Copy link
Author

I was going to say ignore-mutation-of-copy because that seems more self-explanatory, but since they're officially called accessor methods I think the name is good as is.

@RebeccaStevens
Copy link
Collaborator

RebeccaStevens commented Aug 24, 2018

@jonaskello
After sleeping on the PR I made last night, I realised that the following case would fail:

const foo = () => [3, 1, 2];
const sortedFoo = foo().sort();

Although this doesn't use one of the built in array accessor methods, foo is still a function that returns an array just like slice does. Should foo().sort() be allowed?

Pros

  • Consistency

Cons

  • Allows for accidental mutation in non-pure functions. For example:

    const bar = [3, 1, 2];
    const foo = () => bar;
    const sortedFoo = foo().sort(); // oh no, this will also sort `bar`.

    Although this can be protected against by declaring that foo's return type is of type ReadonlyArray<number>

foo().slice().sort(); is always an option though, so maybe this isn't needed.

Should I open a new issue for this?

@jonaskello
Copy link
Owner

I'm trying to think of the use cases for this addition. The current implementation allows the built-in Array methods to be used as they all are immutable, eg. replacing .sort() with .slice().sort() lets us emulate an immutable .sort() method on Array which has many use cases. Maybe one use case for the proposed addition could be if a third party API that returns a mutable array Array. For example, considering that the foo function in your first example would be a third party library function that returns a mutable array and we always want to sort it. I guess that scenario is much less common though but perhaps there are more use cases I have not considered. Opening a new issue with the proposal would probably be a good idea to collect feedback.

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

No branches or pull requests

3 participants