[docs] Remove <any> from nextjs-with-typescript example

[virzak]

• I have followed (at least) the PR section of the contributing guide.

The Use of any cause tslint warnings.
Also adding the use of FunctionComponent.

I wonder if tslint should be added to this example or if at least its rules should be followed for those who would like to add it later on.

[mui-pr-bot]

No bundle size changes comparing c54e545...f2bbcc1

Generated by 🚫 dangerJS against f2bbcc1

[oliviertassinari]

@Janpot By any chance, it would be awesome if you had the time to have a look at the changes. Thank you!

[oliviertassinari]

We never use FunctionComponent in our definitions (NProgressBar exception we should probably change). Is there a reason for changing it here?

[virzak]

This is due to tslint rule which prefers arrow functions to normal functions. The type of props is inferred from the function type.

Wasn't aware of typescript conventions for this project. Is it documented somewhere?

[oliviertassinari]

Does this rule come by default?

[virzak]

Looks like they turn it on for the strictest setting, but the recommended allows named functions too. https://palantir.github.io/tslint/usage/configuration/

[oliviertassinari]

The above seems to be allowed: https://github.com/palantir/tslint/blob/aa2af999e543f80bcd5f637f8c13316f0ecd5e70/src/configs/recommended.ts#L114.

[Janpot]

it should support the strictest tslint rules

I tend to disagree with that. IMO It should compile with strict and be a solid starting point for a mui/ts project. I think if they want to use an opinionated coding style like tslint it should up to the user to adapt the example to their needs.

[eps1lon]

I think if they want to use an opinionated coding style like tslint it should up to the user to adapt the example to their needs.

Agreed. Lint rules for programming errors or bugs: sure. Stylistic lint rules: no. Following the same convention that create-react-app applies.

This doesn't apply to the only-arrow-functions rule since the rationale doesn't apply to function components:

Traditional functions don’t bind lexical scope, which can lead to unexpected behavior when accessing ‘this’.

[virzak]

So what's the rationale for avoiding FunctionComponent<T>? Is this for consistency with the rest of the code? To me FunctionComponent is a clear indication that this isn't just some function, but a react component. As this guide points out, you can have access to the children prop.

It is also being used in one of the best known TypeScript books

Not an issue reverting back to previous syntax, but just curious, since I thought that was the standard.

[Janpot]

I see

import React, { FunctionComponent, PropsWithChildren } from 'react';

interface Props {
  prop: 'hello';
}

// either:
function Component1 (props: PropsWithChildren<Props>) {
  return <div>{props.children}</div>;
}

// or:
const Component2: FunctionComponent<Props> = props => <div>{props.children}</div>;

// or to have displayName
const Component3: FunctionComponent<Props> = function Component3(props) {
  return <div>{props.children}</div>;
};

makes sense to me, but I have no preference.

[eps1lon]

As this guide points out, you can have access to the children prop.

Implicit children should be avoided since they are neither accurate nor likely to stay in the future.

So what's the rationale for avoiding FunctionComponent?

I outlined this above in #16555 (comment). TL;DR: component display names

To me FunctionComponent is a clear indication that this isn't just some function, but a react component.

99% of the time a uppercase first function is a react component in a react codebase. Uppercase first functions I only ever see in codebases that don't use class syntax.

[Janpot]

[email protected] now exposes LinkProps vercel/next.js#7905

[eps1lon]

Would like to accept this with only the ref typing but the stylistic changes are motivated by a lint rule of which the rationale doesn't apply here.

[eps1lon]

This changes the display name in react devtools:

3.6:

- ForwardRef(NextComposed)
+ ForwardRef

experimental:

- <NextComposed> ForwardRef
+ <Anonymous> ForwardRef

Both of which are less ideal.

[virzak]

Can have is this way:

const NextComposed = React.forwardRef<HTMLAnchorElement, NextLinkExtendedProps>(
  function NextComposed(props, ref) {

That way dev tools will show ForwardRef(NextComposed)

Personally I think it is confusing having NextComposed being both a constant and a function, but that achieves the goal if dev tools are higher priority.

[Janpot]

@virzak note #16583

[virzak]

[email protected] now exposes LinkProps zeit/next.js#7905

Thanks for the tip. Using this now. Had to drop the Next.js href property because it was conflicting with MUI Link which uses the underlying AnchorHTMLAttributes

[oliviertassinari]

@virzak Could you remove the usage of React.FunctionComponent from the pull request (and use named function instead)? Thanks :)

[oliviertassinari]

What about the other props of the Link component? variantMapping, noWrap, gutterBottom and underline?

[virzak]

Added all of the missing properties. Not sure if there can be a better solution to & {underline?: 'none' | 'hover' | 'always'};

Should there be a exported prop type for Link that includes underline?

https://github.com/mui-org/material-ui/blob/b198da072707eb6da1b5579b35eba989e26a6be4/packages/material-ui/src/Link/Link.d.ts#L9

[oliviertassinari]

I have tried to make an alternative implementation leveraging LinkProps without any success. Would using this type be more future proof?

[virzak]

Yeah, just to avoid defining it again. I guess it is what it is at this time.

[virzak]

@virzak note #16583

@Janpot thanks, take a look. I think the PRs are similar now.

[eps1lon]

@virzak Thank you for working on this. Much appreciated.

[virzak]

@virzak Thank you for working on this. Much appreciated.

@eps1lon happy to help. Awesome library!