owncast
Mixed formatting in components
Share your bug report, feature request, or comment.
Components have a mix of stylings, import/export patterns, and FunctionalComponent usage.
I propose:
- all components are named exports (as the non-default export)
- all components' props are written as types, not interfaces
- all components' props are named exports
- all components are written with the modern
const Foo = () => ...syntax - components without logic or hooks should return with
() => (<>...</>), if possible - components with props should define their props type by typing the
FCtype, not using the JSX PropTypes lib- this will result in this syntax:
const Foo:FC<FooProps> = ({ a, b }) => ... - n.b.: the
FCtype wasn't always recommended due to the implicit{children: React.reactNode}field, but since React 18, this has been removed andFCis now recommended - JSX's PropTypes is unnecessary in TSX
- this will result in this syntax:
- class components should be marked as
needs refactoringand an issue should be opened to track rewriting them as functional components - event handlers should be prefixed with
on
This is great feedback. I wonder if there's any kind of linting rules that can be added to enforce these best practices and standards. Any kind of tooling to help is always nice.
One thing that's probably clear is all the "admin" stuff is completely different than everything else, and that's because it really is completely different. It was written long ago and was literally a different repository until recently when it got merged in here, so everything about it is all over the place.
Now that the new frontend is being built I'm kind of leaving the admin stuff alone as to not add additional work on top of all the other things that need to get done. Kind of a "if it's not broke don't fix it" kind of deal, but I agree it would be nice to eventually clean up the admin and make it fit the new stuff. Filing todos for those class components and marking them as "good first issue" makes sense so it doesn't get forgotten, thanks for suggesting it.
There's an eslint rule for consistent functional components, but it's disabled in the repo. I'll enable it in my PR and configure it to enforce the code style I suggested above. It won't fix everything but it's a start.
There's also a rule for named exports. I'll see if that can be used.
I've noticed a lot of linting rules being disabled ad-hoc. We might want to define when it's okay to do this. There's a few components with a half dozen rules disabled 😆.
Leaving this open until I copy the above into some document.
I'll add a doc to the repo now that explains the styles we use for components and the rationale behind them.