Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

refactor(linting warnings): Fixed linting warnings #410

Merged
jonchurch merged 1 commit into from
Feb 18, 2020
Merged

refactor(linting warnings): Fixed linting warnings #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/controls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const Controls = ({
lightModeIcon = 'wb_sunny',
darkModeIcon = 'nights_stay',
controller = new DarkModeController(),
}: Props) => (
}: Props): JSX.Element => (
<header css={controlsStyles.header}>
<div id="controls" css={controlsStyles.controls}>
<span>
Expand Down
4 changes: 2 additions & 2 deletions src/components/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ interface Props {
darkModeController?: DarkModeController;
}

const Header = ({ darkModeController }: Props) => (
const Header = ({ darkModeController }: Props): JSX.Element => (
Copy link

@LinusU LinusU Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend typing this as:

const Header: React.FC<Props> = ({ darkModeController }) => (

Not only is it a few characters shorter, but it also tells that the Header is a React functional component, instead of just being a function that happens to conform to the interface of a functional component ☺️

Copy link
Contributor

@jonchurch jonchurch Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id welcome any guidance on typings with React, I dont actually know much about this at all honestly.

What Im seeing here is that because we are using a function expression we should document that expression returns a react functional component function, instead of just documenting the return type of that function itself.

I dont fully understand the benefits of this but I do think I understand the semantic difference.

Thank you for taking a look at this and bringing it up, Im very open to a discussion around the conventions we are using ❤️

Copy link
Member Author

@benhalverson benhalverson Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @LinusU

Copy link
Contributor

@SMotaal SMotaal Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LinusU… react continues to impress me on how they fuzz TypeScript types :)

So I dug this out — https://unpkg.com/browse/@types/react@16.9.22/index.d.ts#L515:

type FC<P = {}> = FunctionComponent<P>;
interface FunctionComponent<P = {}> {
  (props: PropsWithChildren<P>, context?: any): ReactElement \| null;
  propTypes?: WeakValidationMap<P>;
  contextTypes?: ValidationMap<any>;
  defaultProps?: Partial<P>;
  displayName?: string;
}

Sure enough they wrapped an interface with a type as an alias, and sadly that means it will not type … | (props: P) => JSX.Element | (props: P, context: any) => JSX.Element which is really the developer facing type in all of this — sorry, deep breaths :)

@jonchurch here is a good ref for react typing needs — https://github.com/typescript-cheatsheets/react-typescript-cheatsheet/blob/master/README.md#the-types-i-need-werent-exported

Q

do we want to bother retyping everything?

I honestly would not bother if it is not throwing… typing working for our needs, not React strict but still React.

Suggested change
const Header = ({ darkModeController }: Props): JSX.Element => (
const Header: React.FC<Props> = ({ darkModeController }) => (

So if we opt for it we will need to do work all over the place, and that can potentially break things or at least complicate other PRs

Q

do we really need JSX.Element coercion?

I honestly think it is always best to rely on TypeScripts inferred types unless you know for sure there is a glitch or preference… they do a good job nowadays that sometimes it is more accurate than coercion.

Suggested change
const Header = ({ darkModeController }: Props): JSX.Element => (
const Header = ({ darkModeController }: Props) => (

So doing it like this meant TypeScript inferred either JSX.Element or React.Element… not sure I can tell which with certainty as of current, but we can check out the code see what hovers.

@benhalverson — I just unresolved this conversation, let's figure out a game plan here then see if we need to create a new PR, push here and merge, or go the longer "task" issue route.

Sorry for all the edits, I was slowly getting my bearings and things just unfolded… I will try my best to not always do that :)

<nav className="nav">
<div className="logo">
<Link to="/">
Expand Down Expand Up @@ -56,7 +56,7 @@ const Header = ({ darkModeController }: Props) => (
<button
type="button"
className="dark-mode-toggle"
onClick={() => {
onClick={(): void => {
Copy link
Contributor

@SMotaal SMotaal Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benhalverson Can we work on t/eslint together to avoid needing the obvious coercion… I know it is an acquired taste but onClick's typings should allow the typechecker to throw if my function is doing something wrong, and coercion here basically dilutes from having this clarity.

if (!darkModeController)
document.body.classList.toggle('dark-mode');
}}
Expand Down
2 changes: 1 addition & 1 deletion src/components/hero.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ interface Props {

const NodeVersion = 'Version 10.15.3';

const Hero = ({ title, subTitle }: Props) => (
const Hero = ({ title, subTitle }: Props): JSX.Element => (
<div className="home-page-hero">
<h1>{title}</h1>
<h2 className="sub-title t-subheading">{subTitle}</h2>
Expand Down
1 change: 0 additions & 1 deletion src/components/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ const Layout = ({
title,
description,
img,
location,
darkModeController = new DarkModeController(),
}: Props): JSX.Element => {
return (
Expand Down
2 changes: 1 addition & 1 deletion src/pages/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const nodeFeature2 =
const nodeFeature3 =
'Lorem ipsum dolor amet pug vape +1 poke pour-over kitsch tacos meh. ';

const NodeFeature = ({ img, featureText }: Props) => {
const NodeFeature = ({ img, featureText }: Props): JSX.Element => {
return (
<div className="node-features__feature">
<img src={img} alt="node feature" />
Expand Down