Refactor package vulnerability scanner stores #232
Conversation
| @@ -1,51 +1,49 @@ | |||
| { | |||
| "version": 1, | |||
There was a problem hiding this comment.
Most of these are Prettier formatting changes. The only real changes are file checksums
| function handleClick() { | ||
| contentStore.currentContentId = props.content.guid; | ||
| scannerStore.currentContent = props.content; |
There was a problem hiding this comment.
I'm not fully familiar with Pinia yet, so take this comment with a handful of salt. Couple questions here, also for my own education:
- Assigning an object to the state, could it lead to unwanted side-effects? since
props.contentis a reactive object, any change to it would propagate, this can be good when that's the intention though. - Updating the state directly, I think it is pretty handy and simple, instead of having to create methods to mutate the state. Still, is this the encouraged way to do it in Pinia? IIRC the
.$patchmethod is preferred, although I don't know if there is a substantial difference.
There was a problem hiding this comment.
Happy to answer :)
Pinia has examples in the documentation to directly manipulate state. The $patch method is to apply multiple changes at once to prevent duplicate re-renders. If you are manipulating one there isn't a need to do that.
It can lead to unwanted side-effects but in this case I want to react when scannerStore.currentContent changes. One reason I have props.content specifically is so I can guarantee that this component is getting Content for typing reasons (as opposed to grabbing the content from scannerStore.currentContent and having undefined logic everywhere.
References: https://pinia.vuejs.org/core-concepts/state.html#Mutating-the-state
There was a problem hiding this comment.
Sorry I submitted a bit early, edited my comment to have everything I wanted to say.
There was a problem hiding this comment.
Thank you for the explanation! 😄
| v-if="isFetched" | ||
| :type="hasVulnerabilities ? 'error' : 'success'" | ||
| v-else | ||
| :type="content.vulnerabilityCount > 0 ? 'error' : 'success'" |
There was a problem hiding this comment.
nit: this attr check might be better as a computed instance?
| @@ -56,7 +58,7 @@ fetchPackagesInBatches(); | |||
| /> | |||
| </div> | |||
|
|
|||
| <div v-else-if="contentStore.contentList.length === 0"> | |||
| <div v-else-if="scannerStore.content.length === 0"> | |||
There was a problem hiding this comment.
same nit here: I think the other checks you have, where the logic is handled as computed and the template is a bit cleaner, are pretty good.
Also, since this is already data from the store, WDYT about a getter? e.g: scannerStore.hasContent
There was a problem hiding this comment.
Agreed with you on all the comments you left. I'll consolidate the computed properties to scannerStore where it makes sense.
In an effort to improve the Package Vulnerability Scanner with additional features a bit of a store refactor was required to raise information about Content, their packages, and the vulnerabilities related to those packages. Previously this information was split across components and used as needed. Now there is a
scannerstore that collects that information into aContenttype to be used by several components.The new
Contenttype consolidates thepackagesandvulnerabilityCountinfo so it can be used in a cleaner way. This also opens up the door for some more features to be done in a cleaner way.These changes have been published to our internal Dogfood server here.