-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
[prefer-readonly-type] Rework this rule. #153
Comments
@jonaskello Thought's on this proposal? |
I agree with this :-). The rule was created to avoid non-functional stuff from happening. Especially on teams where developers have limited experience with fp. If you cannot mutate the return value then you are left with the option to pass it to another function for transformation or overriding the rule. It helps teams to treat everything as an immutable expression that you can only pass on to some other function. I think it can help teach teams to start thinking more in functional programming terms. If you use a fp language everything is immutable, including return values.
This is also true and if you can use discipline to not mutate the return value (eg. you are somewhat experienced in fp) then the rule is probably not needed.
I agree this is a problem and probably the main reason to consider not using this rule. IMO the rule is good for application code but when doing a lot of integration with 3rd party libs it can become a pain as you have to override it often. I have not had a lot of problem with this in my projects but perhaps we can collect some samples of where it is hard to have this rule enabled?
I'm not sure I follow on this, could you give an example to clarify? |
So it seems like the original rule was designed around the idea of "make everything immutable so we can guarantee that mutations aren't happening", while this new proposal is more around the idea of "make things we are using immutable so that we aren't mutating things, don't care what others do" (I don't think that's the best 1 line description but it will do for now). The "downside" to this new approach is when using your own functions, you need to recast the return values as immutable. For example: // myFunc returns a mutable type.
const mutableFoo = myFunc(1, 2, 3);
const foo = mutableFoo as DeepReadoly<typeof mutableFoo>;
// ... However, this same issue already exist when using 3rd party functions as they also return mutable types. But the advantage is that the 3rd party isn't enforcing any usage constrains on the end user. So this "downside" should really be seen as a good thing rather than a negative. Note: If using this rule with the "immutable-data" rule, then there is no need to recast values like as done above.
Example: type SomeType = Array<SomeOtherType>;
// ^^^^^^^^^^^^^^^^^^^^ - Currently this is marked as an issue, new proposal is to not mark this (by default).
function foo(bar: SomeType): SomeType {
// ^^^^^^^^ - Currently this is not an issue as it is assumed the type alias is immutable, new proposal is to mark this.
// ^^^^^^^^ - Return type isn't marked in either case (using default config).
} |
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…y default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
@jonaskello I'm thinking that it might best to deprecate this rule in favor of @typescript-eslint/prefer-readonly-parameter-types. The core reason for this rule is to do what that rule is doing. The additional things this rule is doing is pretty much things that I would be taking out in the rewrite as talked about above. |
I think I understand better what the proposal is now. So instead of explicitly requiring types/interfaces to be declared with I'm hesitant to remove/deprecate the rules that requires explicit So from the above I think you can have two different approaches to immutability. The first approach would be to explicitly require declaration of readonly types everywhere and when you need mutable types (eg when using 3rd party functions) you need to cast them to non-readonly types (which admittedly is ugly) or mark them as mutable by naming convention. The second approach would be to only require readonly types to be passed into your own functions which could avoid some pain when calling 3rd party functions. I think we should support both these approaches. By enabling both However I can see how the rules Please let me know if I misunderstood anything :-). |
I'm thinking that we might want to add a new rule that deals with ensuring type aliases are readonly/mutable based on some condition. Then that rule in conjunction with For example: |
I made a start on this new rule. Right now the working name is prefer-readonly-type-alias. |
@jonaskello The new rule is mostly done now; just small tweaks to go and waiting on some upstreams. This new rule plus |
I'm trying to understand the implcations of using the Looking at the rule creation code seems like a good place to start: prefer-readonly-type export const rule = createRule<keyof typeof errorMessages, Options>(
name,
meta,
defaultOptions,
{
ArrowFunctionExpression: checkImplicitType,
ClassProperty: checkProperty,
FunctionDeclaration: checkImplicitType,
FunctionExpression: checkImplicitType,
TSArrayType: checkArrayOrTupleType,
TSIndexSignature: checkProperty,
TSParameterProperty: checkProperty,
TSPropertySignature: checkProperty,
TSTupleType: checkArrayOrTupleType,
TSMappedType: checkMappedType,
TSTypeReference: checkTypeReference,
VariableDeclaration: checkImplicitType,
}
); prefer-readonly-type-alias export const rule = createRule<keyof typeof errorMessages, Options>(
name,
meta,
defaultOptions,
{
TSArrayType: checkArrayOrTupleType,
TSIndexSignature: checkProperty,
TSInterfaceDeclaration: checkTypeAliasDeclaration,
TSMappedType: checkMappedType,
TSParameterProperty: checkProperty,
TSPropertySignature: checkProperty,
TSTupleType: checkArrayOrTupleType,
TSTypeAliasDeclaration: checkTypeAliasDeclaration,
TSTypeReference: checkTypeReference,
}
); I'll enumerate the differences here for reference: Removed ArrowFunctionExpression: checkImplicitType,
ClassProperty: checkProperty,
FunctionDeclaration: checkImplicitType,
FunctionExpression: checkImplicitType,
VariableDeclaration: checkImplicitType, Added TSInterfaceDeclaration: checkTypeAliasDeclaration,
TSTypeAliasDeclaration: checkTypeAliasDeclaration, Would this be a correct assessment of the code changes? |
So the main difference will be that variable declarations will not be checked ( Also class properties are no longer checked. |
We can also keep both rules around while we gather feedback from users. |
From what I understand we have this now then: {
// Covered in the same way by new rule prefer-readonly-type-alias
TSArrayType: checkArrayOrTupleType,
TSIndexSignature: checkProperty,
TSParameterProperty: checkProperty,
TSPropertySignature: checkProperty,
TSTupleType: checkArrayOrTupleType,
TSMappedType: checkMappedType,
TSTypeReference: checkTypeReference,
// Covered by @typescript-eslint/prefer-readonly-parameter-types except return types
ArrowFunctionExpression: checkImplicitType,
FunctionDeclaration: checkImplicitType,
FunctionExpression: checkImplicitType,
// Not covered
VariableDeclaration: checkImplicitType,
ClassProperty: checkProperty,
} I think we should keep the original intent of the rule to have everything possible declared as readonly types. The Perhaps the new rule should have a name like |
I think the intent of the rule would be that if you are explicitly declaring something, it should be checked to be a readonly declaration. If you are implicitly inferring a type it should not be checked by this rule. In the implicit case you could either cover it by the |
My intention behind switching to the new rule and My current thoughts are now that we should not deprecate I have also been thinking of introducing a new config/preset in the next major release. Currently we have "recommended" and "lite"; I was think of adding a new "strict" one. This strict preset will focus on enforcing functional linting rules very harshly and I think it would be a good place to include the current But as these rules can't really be used together, maybe it would be best to just create a new rule that merges the two and allows for it to be configure either way. I'll have to do a bit more thinking on this about how such as rule would be implemented. |
Yes, at work we usually do projects in a "functional core, imperative shell" style where the all the core application code is more purely functional and the integration with 3rd party libs are pushed to the corners of the app (as 3rd party libs usually are careless with side-effects). For example for client-side we use typescript-tea which is just an implementation of the elm arch which enables all side-effects (and 3rd party lib integrations) to live in effect managers outside the core application code. I've also experimented with using iterators to emulate algebraic effects to get the same separation on the server-side. So for these environments we would like to lint all application code more strictly as it never calls into 3rd party code directly. I think merging to one rule with more options would be a good idea. If there is overlap with |
Working on the merged rule now. Seems to be coming along pretty well so far. |
…by default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
@jonaskello Merged rule should be pretty much finished now #256 |
@jonaskello I've been thinking that it might be best to rename the rule to |
…by default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
…by default BREAKING CHANGE: allowMutableReturnType is now on by default re #153
Function Return Types
The return type of functions shouldn't be enforced to be immutable (by default) (this behavior can currently be turned on with the
allowMutableReturnType
option).Following from this, the rule
@typescript-eslint/explicit-function-return-type
should no longer be a recommend external rule.Enforce Mutable Return Types
If the return type has to immutable then something very non-functional is going on. What happens to the data after it is returned by a function should be of no concern to the function itself. Mutable return types are more compatible with 3rd-party non-function code as readonly types can not be given as mutable types in TypeScript (while mutable types can be given as either a mutable or readonly type).
An option to enforce this behaviour should be added to the rule (whether this is the default behavior or not is currently undecided).
Type Aliases
Often a mutable type needs to be aliased. This rule should not trigger an error when a mutable type is aliased (by default), only when the mutable type is used.
Care will be need to be taken when implementing this to keep performance good - we don't want to reevaluate every instance of type alias usage to check whether it's mutable or not.
New Rule
As this is such a large change to the current implementation of rule, it's probably best to instead create this change as a new rule and deprecate the current rule. Any suggestions on a new rule name?
The text was updated successfully, but these errors were encountered: