Validation type functionality #153
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
package-lock.json
Outdated
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
Issue: This file can be deleted.
|
|
||
| const fetcher = (url: string) => fetch(url).then((res) => res.json()); | ||
|
|
||
| preload(`https://www.otelbin.io/validation/supported-distributions`, fetcher); |
There was a problem hiding this comment.
Issue: This can now be removed as we are preloading the distros using an instruction in the root layout (has better performance characteristics).
| version: "", | ||
| distro: "", | ||
| const [{ distro, distroVersion }] = useUrlState([distroBinding, distroVersionBinding]); | ||
| const [currentDistro, setCurrentDistro] = useState<ICurrentValidation>({ |
There was a problem hiding this comment.
Question: Why do we need this state?
I would expect that the combination of URL state and the result of useDistributions is sufficient to generate the right label. This would also avoid the need for useMemo and useEffect.
There was a problem hiding this comment.
Exactly today I tried the approach without useState, but at final step, without useState and useEffect, for the first state change, useUrlState couldn't change the whole state of the app, (ex: display current flag, or button title). If you're sure useUrlState is enough, I'll try again. @bripkens
| current: ICurrentValidation; | ||
| setCurrent: (current: ICurrentValidation) => void; | ||
| currentDistro: ICurrentValidation; | ||
| setCurrentDistro: (current: ICurrentValidation) => void; |
There was a problem hiding this comment.
Question: Why do we need setCurrentDistro here? We already have getUrl accessible within the file that we can use to change the distro.
| <Button | ||
| onClick={() => setCurrent({ provider: "Browser-only", version: "", distro: "" })} | ||
| onClick={() => { | ||
| setCurrentDistro({ ...currentDistro, title: { provider: "Browser-only", version: "", distro: "" } }); |
There was a problem hiding this comment.
Issue: There should only be one source of truth for state, i.e. the URL.
| version: (data && Array.isArray(data[key]?.releases) && data[key]?.releases[0]?.version) || "", | ||
| })) | ||
| : []; | ||
| const [{}, getUrl] = useUrlState([distroBinding, distroVersionBinding]); |
There was a problem hiding this comment.
Suggestion: See the SonarCloud bug report on this one. The empty destructuring should be avoided (also in the other location in which useUrlState is used):
|
Kudos, SonarCloud Quality Gate passed!
|
No description provided.