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

Ignoring React components defined as an arrow function #35

Open
jakubmazanec opened this issue May 24, 2022 · 15 comments
Open

Ignoring React components defined as an arrow function #35

jakubmazanec opened this issue May 24, 2022 · 15 comments

Comments

@jakubmazanec
Copy link

Would it be possible to ignore React components in the rule where? While I agree that function declaration is preferable when exporting a function (i.e. export function somefn() { /* ... */}), I would like to make exception for React components, because arrow functions are typed more easily.

When using function declaration, you have to type props and also return (from my experience, people usually forget to add null):

interface BoldProps {
    caption: string;
}

export function Bold({ caption }: BoldProps): JSX.Element | null {
    return <b>{caption}</b>;
}

Using arrow function is little bit more concise, less error prone, but still readable thanks to the React.FC type right after the variable name:

interface BoldProps {
    caption: string;
}

export const Bold: React.FC<BoldProps> = ({ caption }) => {
    return <b>{caption}</b>;
}

Thanks for the info.

@spawnia
Copy link

spawnia commented Jul 25, 2022

I think this proposal can be generalized. I think arrow functions are fine when assigned to a variable with a type declaration.

type Predicate = (value: unknown) => Boolean:

const isNullish: Predicate = (value) => value == null;

@getify
Copy link
Owner

getify commented Jul 25, 2022

I don't plan to add type parsing to these rules. That would need to be another rule that someone else could build.

@spawnia
Copy link

spawnia commented Jul 25, 2022

I think the rule does not necessarily need to parse the type, it just needs to know if a variable has an explicit type declaration. To me, this seems like a reasonable exception for the options global and global-declaration in rule "where".

@getify
Copy link
Owner

getify commented Jul 25, 2022

It's too slippery of a slope. We add an exception for "if a type is present" and then someone wants "well, if the type is like this __".

I am opposed to TS personally, so I'm not motivated to cross the barrier into making this rule type-aware in any sense.

@getify
Copy link
Owner

getify commented Jul 25, 2022

More generally, my reaction to requests to add exceptions for the where of using arrow functions in various contexts is... it seems like mostly, people want to use arrows as general functions in most/lots of contexts -- which is fine, btw... if you like them, use them.

But the more where exceptions are made, the more it waters down the usefulness of this part of the rule. Why even use the where rule at all if there's dozens of nuanced situations where the rule is muted anyway. If the majority of places arrows are allowed, and only a few rarer specific places arrows are disallowed, I don't think where should be used.

I think exceptions should be handled as exceptions rather than as rule configurations. Add an eslint-disable comment if you really feel like using an arrow in a place this lint rule doesn't allow you to.

With respect to the OP specifically, I would suggest just disabling "export" mode, with export: false, of the where rule.

@getify
Copy link
Owner

getify commented Jul 25, 2022

I think arrow functions are fine when assigned to a variable with a type declaration.

To address this opinion, explicitly: I respect your opinion here, but I very much disagree with it. In fact, I'd say it's counter to the underlying philosophy of this rule. The more is involved in an => arrow function expression -- such as adding type definitions -- that doesn't make it more easily readable, I think it adds more visual information to parse at a glance, which actually obscures the arrow function and its important parts.

So making an exception mode for allowing "arrow functions with types" is going in the opposite direction to what this rule is trying to accomplish, which is that it's trying to allow only simpler (visually) arrow functions and it's trying to discourage complex (visually) arrow functions.

@spawnia
Copy link

spawnia commented Jul 25, 2022

In this specific case, TypeScript users are pretty much forced to use function expressions if they want to reuse function types. Given function types can be quite complex or even opaque, it can be very inconvenient to not use them. The inability to declare the type of a function declaration is less of an ad-hoc exception, rather it is a fundamental limitation of TypeScript syntax.

I myself would prefer if TypeScript added syntax to declare the type of a function declaration, perhaps like this:

// Unfortunately not possible in TypeScript
function isNullish: Predicate (value) {
  return value == null;
}

I guess one could also use function expressions instead of arrow functions. Would where allow this?

const isNullish: Predicate = function (value) {
  return value == null;
};

I understand not wanting to make this package TypeScript-specific at all, it really is a slippery slope. The reality is that more and more people are using TypeScript though. Perhaps someone needs to make a TypeScript-aware variant of the rules - I have seen this for other rules that can be improved/modified due to the extra information afforded by types.

@getify
Copy link
Owner

getify commented Jul 25, 2022

The inability to declare the type of a function declaration is...

This is legal in TS, AFAICT:

function isNullish(value: unknown): Boolean {
  return value == null;
}

I understand that's not a "reusable" function type signature. But that's just a tradeoff that TS forces. Moreover, IMO, function return types should be explicit on the function rather than generically re-used.

@spawnia
Copy link

spawnia commented Jul 25, 2022

Of course it is possible to just declare the type of the function inline. In this contrived example, this is probably fine - and it will compile without issues due to TypeScript having a structural type system.

However, I would argue there are definitely cases where it can be advantageuous to reference named function types. The name can communicate meaning, and the reusability aspect can reduce overall complexity and code duplication.

I am curious what you think about the second example I provided in #35 (comment). I like having the keyword function in there and actually think it is superior over arrow functions. As long as that is allowed, I think the conflict between the rule where and the limited nature of TypeScript can be resolved by using function expressions. Anyway, thanks for the good great discussion.

@getify
Copy link
Owner

getify commented Jul 25, 2022

As long as that is allowed...

If I understand you correctly, you're asking about this function form?

const isNullish: Predicate = function (value) {
  return value == null;
};

That's not an arrow function, so no part of this lint rule will ever say anything about it.

@jakubmazanec
Copy link
Author

I think arrow functions are fine when assigned to a variable with a type declaration.

To address this opinion, explicitly: I respect your opinion here, but I very much disagree with it. In fact, I'd say it's counter to the underlying philosophy of this rule. The more is involved in an => arrow function expression -- such as adding type definitions -- that doesn't make it more easily readable, I think it adds more visual information to parse at a glance, which actually obscures the arrow function and its important parts.

So making an exception mode for allowing "arrow functions with types" is going in the opposite direction to what this rule is trying to accomplish, which is that it's trying to allow only simpler (visually) arrow functions and it's trying to discourage complex (visually) arrow functions.

While I agree with you, I would still prefer more pragmatic approach. @spawnia very nicely explained why in some places it is really useful to reuse function type aliases (typing arguments and returns is repetitive and can lead to subtle erros). I can't avoid using them, but in other cases I would still like to enforce using function declarations.

@getify
Copy link
Owner

getify commented Jul 25, 2022

I would still prefer more pragmatic approach

Several features/modes of this rule-plugin have been added after extensive discussion and lobbying by users. I am pragmatic enough to be willing to consider and discuss any ideas.

But there's a core philosophy driving this project, and I don't think it serves anyone well to water that core philosophy down, at least not without a compelling argument.

Thus far, "I want.." and "It'd be nice to.." style justifications have been advanced. I don't feel those overcome the philosophical objection I have to adding such a rule-exception. Yet. I have firmly said I don't have any intention to make this rule "type aware", but beyond that, I haven't given an absolute "no". Just a, "this doesn't convince me."

@getify
Copy link
Owner

getify commented Aug 27, 2022

FWIW, I think TypeScript 4.9 satisfies feature might allow applying the reusable type to function declarations: https://twitter.com/leeerob/status/1563540593003106306?s=20&t=giRGfGFeOAIii5Q1upsHyg

@jakubmazanec
Copy link
Author

Unfortunately satisfies cannot be used with function declarations, see microsoft/TypeScript#51556.

@getify
Copy link
Owner

getify commented Feb 27, 2023

...yet. Looks like TS unfortunately dropped the ball on that one.

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