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

type-declaration-immutability: ignorePattern doesn't work on member name #599

Closed
adrian-gierakowski opened this issue Mar 26, 2023 · 11 comments · Fixed by #683
Closed

type-declaration-immutability: ignorePattern doesn't work on member name #599

adrian-gierakowski opened this issue Mar 26, 2023 · 11 comments · Fixed by #683
Labels
Resolution: Won't Fix A real bug or issue, but the issue is not impactful enough to spend time on. Status: Released It's now live. Type: Feature New features or options.

Comments

@adrian-gierakowski
Copy link

Bug Report

ignorePattern option of type-declaration-immutability doesn't work on member names, contrary to what is advertised here

Expected behavior

given following config:

module.exports = {
  root: true,
  parser: '@typescript-eslint/parser',
  plugins: ['@typescript-eslint', 'functional'],
    rules: {
      'functional/type-declaration-immutability': [
        'error',
        {
          ignorePattern: ['^mutable'],
        }
      ],
    },
    parserOptions: {
      project: ['./tsconfig.lint.json'],
    },
}

there should be no linter error for the following code:

type Person = {
  readonly name: string;
  mutableAge: number;
};

Actual behavior

The following error is raised:

1:6  error  This type is declare to have an immutability of at least "Immutable" (actual: "Mutable")  functional/type-declaration-immutability

Note that the following does not cause an error, which means that the ignorePattern is in force, but only matches the type name, not it's members.

type mutablePerson = {
  readonly name: string;
  age: number;
};

Steps to reproduce

Run eslint on the above code using the above config.

Proposed changes

Mutability checks for members which names match ignorePattern should be skipped

@adrian-gierakowski adrian-gierakowski added Status: Triage This issue needs to be triaged. Type: Bug Inconsistencies or issues which will cause a problem for users or implementors. labels Mar 26, 2023
@RebeccaStevens RebeccaStevens added Resolution: Won't Fix A real bug or issue, but the issue is not impactful enough to spend time on. Type: Feature New features or options. and removed Type: Bug Inconsistencies or issues which will cause a problem for users or implementors. Status: Triage This issue needs to be triaged. labels Mar 26, 2023
@RebeccaStevens
Copy link
Collaborator

The ignorePattern only works on the alias name, not on any of its members (#467).

If we were to add support for ignoring members, we'd likely need do it in a new option.
Though, I don't think it's very likely that this will be add.
In the example given, I would not be in favor of letting Person be considered immutable when it is very much mutable.

@RebeccaStevens RebeccaStevens closed this as not planned Won't fix, can't repro, duplicate, stale Mar 26, 2023
@adrian-gierakowski
Copy link
Author

adrian-gierakowski commented Mar 27, 2023

The example above was taken verbatim from your own documentation, so maybe this page should be deleted, or edited?

@jonaskello
Copy link
Collaborator

jonaskello commented Mar 27, 2023

I think the prefer-readonly-type worked according to the docs. However it is now superseded by type-declaration-immutability.

The original intent of field level mutability for prefer-readonly-type was to mimic the approach some immutable-first languages takes to mutability. They usually have a mutable modifier instead of a readonly modifier on the field level. For example F# and ocaml has the mutable modifier on records.

I guess the reason for having anything mutable is performance, and to be honest I seldom use this ignore feature of eslint-functional myself. However since it has been available before I think it would be interesting to know how widely used it is so I'll re-open so we can collect some opinions on this.

Of course, it would be nice if typescript could be compiled with a --readonly-first flag and instead of using readonly everywhere we could just use mutable for a few fields. However I don't see this being added anytime soon to the compiler :-).

@jonaskello jonaskello reopened this Mar 27, 2023
@RebeccaStevens
Copy link
Collaborator

The example above was taken verbatim from your own documentation, so maybe this page should be deleted, or edited?

Yeah, I should have re-written that file with the release of v5.

The reasoning behind my last comment and closing this issue was based off the fact that with v5, the rules type-declaration-immutability and prefer-immutable-types take a different philosophy to the v1-v4 prefer-readonly-type.
I don't think I've fully documented this philosophy properly and how it differs from the previous one. I'm not exactly sure how to best document it though.

@RebeccaStevens
Copy link
Collaborator

Note: Once @typescript-eslint/type-utils v6 comes out, I'll be making some improvements to is-immutable-type using the new functionality coming (#575 and #577 are waiting for this).
This update will likely create some breaking changes, so I'll probably a make new major release of this plugin then (#598 will likely get included with in).

@rickmed
Copy link

rickmed commented Jul 21, 2023

So let's say I have a class (or object). Is there any way to do this?

class Hey {
    mut_array = []
}

const hey = new Hey()

hey.mut_array.push(1)   // currently, it complains with "Modifying an array is not allowed."

@RebeccaStevens
Copy link
Collaborator

@rickmed I think you replied to the wrong thread. #691 (comment)

@rickmed
Copy link

rickmed commented Jul 23, 2023

@RebeccaStevens don't think so. The other issue is about const/let functionality.

@RebeccaStevens
Copy link
Collaborator

Ahh, sorry. I thought this was just a follow up question for over there. I'll move my reply here.

@RebeccaStevens
Copy link
Collaborator

So let's say I have a class (or object). Is there any way to do this?

class Hey {
    mut_array = []
}

const hey = new Hey()

hey.mut_array.push(1)   // currently, it complains with "Modifying an array is not allowed."

You can use the ignoreAccessorPattern option for this.

This should do what you want:

{
  "ignoreAccessorPattern": "**.mut_*.**"
}

RebeccaStevens added a commit that referenced this issue Jul 30, 2023
@github-actions
Copy link

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the Status: Released It's now live. label Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Won't Fix A real bug or issue, but the issue is not impactful enough to spend time on. Status: Released It's now live. Type: Feature New features or options.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants