That utility function takes a lot of arguments and someone suggested to refactor arguments as typed object so we get some extra typechecking here.
Details
Typeahead without keyboard support works.
Final tests performed in last diffs.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
web/utils/typeahead-utils.js | ||
---|---|---|
73–80 |
web/utils/typeahead-utils.js | ||
---|---|---|
73–80 | In linked diff the comment mention props that have to be read-only (one of React principles), but do why do we need to do it in utility functions, if everything is passed by value (and not by reference) anyway. It feels like a good practice, so we don't accidentally reuse param as variable inside of function, but is there more meaning to it? | |
74–76 | Return value is memoized later on and there's a lot of stuff in InputState. I had some problems with infinite rerender loops, when a lot of things depended on entire InputState, changed other part of it and then recalculated. That's why I decided to only depend on parts of InputState function really uses |
web/utils/typeahead-utils.js | ||
---|---|---|
73–80 | The more precise your types, the more type errors you catch. We want to encourage developers to not only answer the question of what properties an object has, but to also answer the question of whether those properties are read-only or not We default to $ReadOnly because that's more narrow, and only make things non-$ReadOnly if we need to. It's the same reason more JS defaults to const |
web/utils/typeahead-utils.js | ||
---|---|---|
74–76 | I see, thank you for explaining! |
web/utils/typeahead-utils.js | ||
---|---|---|
75–76 ↗ | (On Diff #20388) | If we don't care about returned value it is better to type it as mixed because that makes the API more convenient. But in this case it seems like we should prefer SetState<T> type. |
73–80 | Another thing is that functions are easier to use if they assume as little as possible about the context in which they are called. If we mutate arguments a caller can't safely use the variables later, as they might have changed. Also pure functions are easier to understand, maintain and test. Marking properties as readonly allows this to be checked by type system. |
web/utils/typeahead-utils.js | ||
---|---|---|
75–76 ↗ | (On Diff #20388) | SetState feels fine, but in input-state.js we're not using it at all. What convention should we use than? +setDraft: (draft: string) => void, +setTextCursorPosition: (newPosition: number) => void, +setTypeaheadState: ($Shape<TypeaheadState>) => void, |