Move all the types to /types directory #1997
Conversation
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1997 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
🟩 priority: low
|
Size Change: -533 B (0%) Total Size: 823 kB
ℹ️ View Unchanged
|
My sense of this is to complicate this issue too much. If a type is derived directly from a constant, have it exported from wherever the constant is exported. They're likely used in tandem. Whether they're in I've also wondered and asked if we'd be able to get rid of a lot of those |
|
We need the constants to use in Vue prop validators: |
|
@obulat I think a while ago we'd discussed doing away with validators when we have TS-ified every usage and prop-site of those particular instances. But you're right, we would need them when we don't have TS (is much left of that, though?) I wish I could find where we discussed this, I think it was in a PR several months ago... |
We have decided to do away with validators, but then we've had problems with the peaks values for Audio being invalid, and debugging was easier because we had runtime validators: #1148 . The way I understand it now, for values that come from the API, having validators that run at runtime is still important. For other values, just using TS without unnecessary type assertions (not using |
|
I see, that makes sense! I wonder if running expectations validations on the data as it comes back from the API rather than in the components is another option then, so that you're not having to keep track of what is API data and what isn't when you're writing components. Even the peak normalisation that is happening in the PR you linked, it seems like something that is more suited to the API response transformations that we already maintain rather than munging that should happen in the components themselves 🤔 That way we can do away with validators but still have expectations proven/enforced against the data coming back from the API... though, I don't wonder if in our case, given we control both, we'd be better off auditing the frontend expectations and making sure they match the APIs... I don't know, no concrete suggestions here, just thinking out loud 🙂 Thanks for reminding me about the occasional benefits of the validators. |
|
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was updated 5 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
Fixes
Fixes #1161 by @sarayourfriend
Description
This PR moves all the type declarations from
/modelsto/types.There are several arrays in the
/typesnow that are not really types, and they are used to create types liketypeof something[number]. Would it make sense to extract them tomodelsso that when you can safely useimport type {something}when you import something from/types? Or is it better to keep related items in the same file, even if they are type and non-type values (as is here)?Testing Instructions
The CI should pass.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin