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

Suggestion: readonly method #22315

Open
Conaclos opened this issue Mar 4, 2018 · 9 comments
Open

Suggestion: readonly method #22315

Conaclos opened this issue Mar 4, 2018 · 9 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@Conaclos
Copy link

Conaclos commented Mar 4, 2018

Problem:

A method (function written with the ES6 method syntax) can currently be replaced with another function by a simple assignment.

As an example consider the following snippet of code:

class Person {
    constructor(...) {...}
    displayName (): string {...}
}
const x: Person = new Person(...)
x.displayName = function(this: Person): string {...} // Assignment

Because the readonly modifier is not usable for methods, this is not possible to prevent this kind of assignments.

Proposal:

A method is always readonly.

The following codes are identical:

class Person {
    displayName (): string {...}
}
class Person {
    readonly displayName: (this: Person) => string = function () {...}
}

Compatibility:

This is a breaking change. However, method syntax is recent and mostly used in classes. Codes which assign a function to a method are certainly rare.

Temporary workaround:

Do not use method syntax in your classes and interfaces. Note hat this leads to very verbose codes.

If you use an interface, the verbosity is acceptable. However you get also strict variance.

interface PersonI {
    readonly displayName: (this: Person) => string
}
@mhegazy mhegazy added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Mar 5, 2018
@feeddageek
Copy link

feeddageek commented Jun 27, 2018

I'd like to add the the Readonly mapped type currently work on method event if the readonly keyword it self is not allowed.

class SomeClass {
    //public readonly SomeMethod() {} // 'readonly' modifier can only appear on a property declaration or index signature.
    public readonly SomeOtherMethod() {}
}
const someObject =  new SomeClass();
someObject.SomeOtherMethod = () => {}; // OK
const someOtherObject = new SomeClass() as Readonly<typeof SomeClass>
someOtherObject.SomeOtherMethod = () => {}; // Cannot assign to 'SomeOtherMethod ' because it is a constant or a read-only property.

A flag under the strict umbrella to make all method readonly by default and force the use of property function for "assignable method" would also be welcome.

@patrykuszynski
Copy link

@feeddageek It's allowed when you store method as arrow function class property:

class SomeClass {
    public readonly SomeOtherMethod = () => {};
}

const someObject =  new SomeClass();
someObject.SomeOtherMethod = () => { /* new impl */ }; //  error TS2540: Cannot assign to 'SomeOtherMethod' because it is a read-only property.

@Conaclos
Copy link
Author

Conaclos commented Jul 15, 2019

Another workaround (based on @feeddageek idea) is to enforce the use of a factory that returns a "readonly version" of the object:

class Person {
  /** Factory */
  static readonly from: (name: string) => Readonly<Person> = (name) => new Person(name)

  name: string

  protected constructor(name: string) {
    this.name = name
  }

  displayName(): string {
    return name
  }
}

const p = Person.from("Louise")
p.displayName = () => "Impersonation" // does not compile :)

Note hat all properties are also made readonly.
However, if TS beginners read this code they could think that the object cannot be mutated (this is not the case if the object has mutating methods).

@feeddageek
Copy link

feeddageek commented Jul 17, 2019

@patrykuszynski the problem with using readonly function property is that they are not compiled on the prototype, they are build into the instance by the constructor.
Also they capture their this and traditional method don't

class Test {

    constructor(public property: string) {}

    public inProto() {
        return `inProto: ${this.property}`;
    }

    public inConstructor = () => {
        return `inConstructor: ${this.property}`;
    }
}

const test1 = new Test('Object 1');
const test2 = new Test('Object 2');

console.log(test1.inProto === test2.inProto); // true
console.log(test1.inConstructor === test2.inConstructor); // false


test2.inProto = test1.inProto; // this does nothing since `test1.inProto === test2.inProto`
test2.inConstructor = test1.inConstructor;
console.log(test2.inProto()); // inProto:  Object 2
console.log(test2.inConstructor()); // inConstructor:  Object 1

const notCapture = test1.inProto;
const capture = test1.inConstructor;
console.log(notCapture()); // inProto: undefined
console.log(capture()); // inConstructor:  Object 1

(the capture of this could be prevented by the use of function expression instead of arrow function, but that's not the point)

errata: test2.inProto = test1.inProto; actually does something, but it is not really relevant to the matter at hand.

@kwasimensah
Copy link

kwasimensah commented Apr 24, 2020

This is also an issue with namespaces. allowing const/readonly would make this less verbose and making it the default would stop library authors from being surprised if an implementation gets swapped out.

namespace Foo {
    export function doSomething() {
        console.log("Hello World");
    }
}

Foo.doSomething = () => { console.log("Overriden") }
Foo.doSomething(); // Prints "Overridden"

namespace SafeFoo {
    export const doSomething =  () => {
        console.log("Hello World");
    }
}
SafeFoo.doSomething = () => { console.log("Overriden") } // Becomes an error.

@kwasimensah
Copy link

kwasimensah commented Apr 24, 2020

This would however break declaration merging/module augmentation since you can't define functions in an ambient context.

declare module global {
    export namespace Foo {
        const doSomethingElse : () => void;
    }
}

// Defining this function in-place isn't allowed either.
Foo.doSomethingElse = () => { console.log("Do Something else."); } // errors

@thw0rted
Copy link

thw0rted commented Dec 8, 2021

FYI, I filed #47003 which is basically the same request, and Ryan replied with a pointer to this issue and a comment that there hasn't been much interest so at this point action is unlikely. It occurred to me that the bar is probably lower to make a linter rule, which I think could be just as effective at catching accidental assignments as a language-level "readonly" modifier. I haven't asked yet but if I do I'll make sure to ping this issue.

@adrian-gierakowski
Copy link

I'd really like to see this happen as currently using classes with one or more methods as a function argument lead to a linter error when https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/prefer-readonly-parameter-types.md is enabled.

It's also quite inconsistent that a class property can be made immutable but a method cannot

@Conaclos
Copy link
Author

Conaclos commented May 9, 2024

Related discussion: #58296 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants