-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Frontend data validation #634
Conversation
@@ -33,10 +33,15 @@ export interface CollectionIndexData { | |||
symbol: string; | |||
} | |||
|
|||
export enum CollectionVisibility { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, since the excluded plugin file on s3 has the invalid
value for visibility, would it make sense to add it in CollectionVisibility
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm interesting, @neuromusic could you comment on this? i'm going based on the validation logic defined on the collections repo, and there's no invalid
value:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk what invalid
means here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify: I'm not sure why a plugin would have invalid
as a value for visibility: https://github.com/chanzuckerberg/napari-hub/wiki/Customizing-your-plugin's-listing#visibility
@@ -17,7 +17,7 @@ export const DEFAULT_PLUGIN_DATA: PluginData = { | |||
python_version: '', | |||
release_date: '', | |||
report_issues: '', | |||
requirements: '', | |||
requirements: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codemonkey800 I left some comments regarding this file in #636 - curious what your thoughts are on duplication of that and potential for the typing to get out of sync. Is there a "source of truth" we could have defined somewhere in a more shared location? Even if this is duplicated in two areas, let's make sure they're working off the same expectations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I raised similar concerns in the beginning and wondered if we could use something like protobuf, but I think we went with JSON because it was quicker to do. I'd love to have shared typing via protobuf or some other standard format, but maybe we should defer that to another ticket because it'd be large-ish effort to complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some type of (shared) JSON file works fine as well! Not familiar with protobuf. I'm good with tackling this in another ticket though; do you mind opening up an issue for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created an issue here: #646
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not going to be the best review since i havent touched this code in a while but generally LGTM, one nitpick on where a test generation function belongs
Description
Adds client-side validation for backend data on the frontend using the zod package. This will allow us to consume backend data safely and provide fallbacks for when data is invalid, enabling us to fail more gracefully.
Plugin data has way more leeway and provides more fallbacks and sanitization than collections. This is because plugin data is directly from PyPi, so we need to graceful with throwing errors when data is invalid.
However, collection data will throw an error if data is invalid. Since we have more control over the data source for collections, and since we also have validation built into the repo, it makes more sense to be a bit more strict on invalid data. We can implement a more graceful approach in the future if that behavior is also desired.
Demos
Error that could happen from collection page if data is invalid: