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

[no-object-literal-type-assertion] Allow for function properties option ("defaultProps") #519

Closed
G-Rath opened this issue May 13, 2019 · 7 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@G-Rath
Copy link
Contributor

G-Rath commented May 13, 2019

Repro

There is no way to directly annotate the type of a function property.
A very common use case of as with a function property is the React defaultProps property:

import React from 'react';

interface Props {
  myProp: string;
}

const Comp = (props: Props) => {
  return (
    <div>
      {props.myProp.toUpperCase()}
    </div>
  );
};

Comp.defaultProps = {
  myProp: 'hello world'
} as Pick<Props, 'myProp'>;

const comp = <Comp />;

It would be reasonable to support allowing as for objects when they're being assigned to function properties.

@G-Rath G-Rath added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 13, 2019
@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for maintainers to take a look labels May 13, 2019
@bradzacher
Copy link
Member

I don't agree with this enhancement for a few reasons:

  1. it's a very, very specific use case.
  2. we'd have to introduce type information to the rule, which is a breaking change
  3. it has two simple workarounds:
const defaultProps: Pick<Props, 'myProp'> = {
  myProp: 'hello world'
}
Comp.defaultProps = defaultProps;
function Comp(props: Props) {
  ////////
}
namespace Comp {
  export const defaultProps: Partial<Props> = {
    myProp: 'hello world'
  };
}

That being said, I won't block on it.

@G-Rath
Copy link
Contributor Author

G-Rath commented May 13, 2019

it's a very, very specific use case.

I don't think it's "very, very specific", given how popular React is, and that with the release of hooks functional components are being used more and more.

we'd have to introduce type information to the rule, which is a breaking change

Yup that's a big change alright. I'd had hoped that wasn't required, but now that I look into I see the matcher is smaller than I expected (as it's just looking for as being used on objects, rather than on objects being assigned).

it has two simple workarounds:

The first workaround was how I was working around this original, but I had forgotten about the second!
I'll give it a try to see how it works with TS-React (i.e if it picks up on defaultProps properly).


After typing the above, I then looked at the implementation of this rule, which seems to already use types?

return {
'TSTypeAssertion, TSAsExpression'(
node: TSESTree.TSTypeAssertion | TSESTree.TSAsExpression,
) {
if (
allowAsParameter &&
node.parent &&
(node.parent.type === AST_NODE_TYPES.NewExpression ||
node.parent.type === AST_NODE_TYPES.CallExpression)
) {
return;
}
if (
checkType(node.typeAnnotation) &&
node.expression.type === AST_NODE_TYPES.ObjectExpression
) {
context.report({
node,
messageId: 'unexpectedTypeAssertion',
});
}
},
};
},

And sure enough:

Error while loading rule '@typescript-eslint/no-unnecessary-type-assertion': You have used a rule which requires parserServices to be generated.

This would mean this no longer requires a breaking change.

Since this rule already supports allowAsParameter, I don't think it would be too much of an extension to support either:

  1. The proposed allowAsFunctionPropertyAssignment option
  2. A "allowedWhenAssigningTo": ["defaultProps"] option
  3. A allowAsPropertyAssignment option

These are in order of preference, since the 3rd option would be rather wide, but less specific.

@j-f1
Copy link
Contributor

j-f1 commented May 13, 2019

Alternatively, you can use native destructuring defaults to replace defaultProps.

@bradzacher
Copy link
Member

After typing the above, I then looked at the implementation of this rule, which seems to already use types?

getParserServices is the keyword you're looking for when looking to see if a rule uses typescript type information. (specifically getParserServices(context))

Error while loading rule '@typescript-eslint/no-unnecessary-type-assertion'

Wrong rule.


There is some duplication of assertion rules which needs to be cleaned up (#501). This rule specifically exists to disallow an explicit cast of an object type (as explicit casts of object types applies more relaxed rules than an implicit type cast, possibly leading to bugs).


@j-f1 - every time I try this - it doesn't work for me...
image

@G-Rath
Copy link
Contributor Author

G-Rath commented May 13, 2019

getParserServices is the keyword you're looking for

Useful to know - thanks 👍

Wrong rule.

Gosh darn it. I checked it like three times to make sure - sorry guys!

@j-f1 destructuring is not the recommended way of doing this in TS, as it causes a performance hit (since the default values are evaluated every call).

@j-f1
Copy link
Contributor

j-f1 commented May 20, 2019

@j-f1 - every time I try this - it doesn't work for me...

Since the name prop has a default value, you have to specify that it’s optional. TypeScript is then smart enough to know that the prop is always defined in the body of the function.

@j-f1 destructuring is not the recommended way of doing this in TS, as it causes a performance hit (since the default values are evaluated every call).

Do you have evidence that this is a significant performance hit? Not destructuring sounds kind of like premature optimization.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 11, 2019
@bradzacher
Copy link
Member

it causes a performance hit (since the default values are evaluated every call).

If your app is concerned with the performance hit of initialising 1 string per render, then your app must be perfectly coded, and is in the 0.0001% of cases; you should be commended on how performant your app.

Sarcasm aside, default values for props is the recommended way of doing it as it optimises for developer experience in exchange for a very, very negligable perf hit.

Speaking from first-hand experience, the Facebook codebase uses object destructuring default values in its functional components, and they operate at the scale of tens if not hundreds of thousands of components.

Considering how perf-conscious they are (you should see the perf tooling that gets automatically run and reported for every PR - every team has size and render budgets), I think that it's fine.

If you're worried about the cost of initialising a string, feel free to rejig your components a bit so it's a prop lookup instead (which should more or less only cost 1 cycle for the memory lookup):

const defaultProps = { myProp: 'hello world' };
const Comp = ({ myProp = defaultProps.myProp }) => {
  // ...
}

As an aside as well that I thought of whilst writing the above.

Using object destructuring allows you to take advantage of performance tricks provided by the engine. (If you're transpiling destructuring statements) You don't know for certain that the string will be re-evaluated every time, nor do you know the tricks the engine has done under the hood to decide if it should use the default value.

Where as with default props you know that the string was evaluate once, but you know the exact algorithm react is using every render, which has its own perf considerations and can only be optimised so much by the engine.

Object.keys(defaultProps).forEach(k => {
  if (!k in prop[key]) {
    prop[key] = defaultProps[key];
  }
});

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

3 participants