inline strings used for app states instead of reusable constants/enums (”Idle”, “Loading”, "Act::Fetch" , etc).
strongly typed approach of the application and the usage of dicriminated union makes using constants unnecessary.
I will discuss it with three examples what I mean.
- We can't have silly typos and autocomplete of IDEs will predict them correctly too.
So for example if we type "Load" the IDE will autocomplete as "Loading".
And when we type something that is not part of the union the compiler will alert us
in that spot.
- If a naming change occurs we are also forced to handle the change. So, it means we don't have the possibility to skip renaming where they are used.
As an example if you change "Loading" state to "isLoading" the compiler gives errors such as:
./src/app/App.tsx
[tsl] ERROR in /Users/aykutyaman/Applications/xite-videos/src/app/App.tsx(19,10)
TS2678: Type '"Loading"' is not comparable to type '"Idle" | "isLoading" | "Videos"'.
@ ./src/index.tsx 15:0-28 20:75-78
[tsl] ERROR in /Users/aykutyaman/Applications/xite-videos/src/app/App.tsx(26,19)
TS2345: Argument of type 'Loading' is not assignable to parameter of type 'never'.
- We can argue that using constants has still an advantage and that is when we want to refactor the strings: the change would be done only in one place. But in this case we'll have inconsistent naming such as constant name Loading but it's value isLoading. So, I think we'll feel to change the constant name where it's used
App.tsx: assertNever error handling in render method doesn’t notify the user in any way that something went wrong. It displays a black screen.
It is used only with discriminated union and it's an unreachable branch.
For example if we try to remove a union's constituent from the switch case the error
will occur but not at the runtime. This usage assumes that any value is not used
in the application. (I think using unknown instead of any is a better approach)
Throwing an error is for protecting against the usage of any.
We could use ErrorBoundary wrapped around the App component. Yes, this permits us the
possibility to handle the error in the case of any. But again the application has strict
TypeScript usage and non any any. :)
const App = (): JSX.Element => {
let state = useSelector<RootState, RootState>(identity);
state = "foo" as any;
...
// matching component
...
return <>compoment</>
}
export default () => {
return (
<ErrorBoundary>
<App />
</ErrorBoundary>
);
};
I know that it seems that I've decided deliberately on some rules. In a team these choices will be made discussing together.
I could use a pattern matching library such as ts-pattern to reduce
the verbosity and mutation. I used a switch/case mostly to have the possibility to
discuss. But in a production ready application something like ts-pattern would be a
better choice:
const App = (): JSX.Element => {
const state = useSelector<RootState, RootState>(identity);
return match(state)
.when(isIdle, () => <Idle />)
.when(isLoading, () => <Loading />)
.when(isVideos, () => <Videos />)
.exhaustive();
};
That been said, I should have turned the no-explict-any from warning to error in .eslintrc but there will still be an escape-hatch with eslint-disable-line etc.
For a further discussion: Stack Overflow answer Type Safe and Exhaustive ‘switch’ statements, aka Pattern Matching in TypeScript
I tried to answer to this in "inline strings used for app states instead of reusable constants/enums (”Idle”, “Loading”, "Act::Fetch" , etc)."
My feeling is that the code is overly complex and verbose, with lots of unnecessary guards against things that can be avoided easily (e.g using strong typing).
We can trust our own TypeScript types, but we can never trust any data coming into our app from the outside. Honestly, I can't see any unnecessary usage of type guards. I've put them in the boundaries of the application. I think some of the boundaries between are client/server, redux/application code, user-input/application code etc.
Also, the verbosity of defining custom guards could be removed using a tool such as io-ts, ts-runtime etc. I think in a production app it would be the choice. I considered here custom guards as a way of demonstration. And a library would have been put magic, and prevent the discussion.
I agree but that I should have displayed to the user with an error state. But after managing all error cases properly it's a trival issue. And the only think to do is to changing the state such as:
type Error = HTTPError | InvalidJSON | IOError[] // These are already defined and populated
export type State = Idle | Loading | Videos | Error
I thought that managing errors was enough to show my approach. For example:
There are already a lot of TODO comments and refactoring reminders in the code, which makes it feel a bit half-baked (even though the app functionalities work great). Personally, I’d like to see a smaller, simpler app that’s polished than a large codebase that seems a bit work in progress.
I agree with this statement. The naming is one the most difficult things in programming. :) However, I think it could be solved by a consensus of conventions with whole team.
And I didn't want to use mocking etc. Dependency Injection would have solved the mocking issue, but in this case I would have to provide the dependency in somewhere. Which means subscription to the store in the parent. I don't have a strong opinion if we should only use hooks, or HOCs. It's a well know pattern even though it seems a little bit advanced. I don't follow blindy when a pattern/tech becomes popular.
Upps! You're right it's unnecessary because state is already narrowed to the Videos and we don't need to use the switch case here.
learning curve. I think its use should be motivated by a real need here, which I don’t see. I’m pretty sure if redux-toolkit was used, 90% of the code related to state management could be removed.
redux-observable, redux-thunk and redux-loop. I haven't used thunks because visualsing the dependecy tree of effects becomes a nightmare when the codebase becomes complicated. One can argue that we can start with thunk and refactor it in future to use other approaches. I think for effect system it becomes almost impossible to the nature of dispatching effects in this way. At my last job there was an admin project named v4-editor. Developers had to rewrite four times the application. And I think the main issue was with throwing action from everwhere. The approach of saga, observable and loop makes this dependency explicit. (Actions in/Actions out) That been said the choice of redux-observable is discutable.