DOP-5036 Refactor Search-Manifest integration #10
Conversation
✅ Deploy Preview for search-manifest-integration ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for redoc-integration canceled.
|
✅ Deploy Preview for persistence-module-integration canceled.
|
✅ Deploy Preview for site-links-display canceled.
|
✅ Deploy Preview for git-changed-files-plugin-bianca canceled.
|
✅ Deploy Preview for snooty-cache-plugin canceled.
|
| }, | ||
| }, | ||
| }; | ||
| // const deleteResult = await collection.deleteMany( |
| this.documents = []; | ||
| this.global = includeInGlobalSearch; | ||
| } | ||
| constructor(url = "", includeInGlobalSearch = false) { |
There was a problem hiding this comment.
What happens if the url property is empty? Does this cause breaking issues down the line?
There was a problem hiding this comment.
Nothing, it should always be empty upon initialization because the value will be returned from getProperties and then set to the existing manifest later on
| this.category = category; | ||
| this.value = value; | ||
| this.subFacets = subFacets; | ||
| if (subFacets) { |
There was a problem hiding this comment.
We don't need this if statement as subFacets should be defined based on it's type definition.
There was a problem hiding this comment.
actually, subFacets should be an array of facets or null, thanks for catching this!
| if (subFacets) { | ||
| for (const subFacet of subFacets) { | ||
| this.subFacets.push( | ||
| new Facet(subFacet.category, subFacet.value, subFacet.subFacets ?? []) |
There was a problem hiding this comment.
If the subFacets array already contain an array of Facet objects, is there a need to create more? It looks like we might be creating duplicates since we push onto this.subFacets which has subFacets assigned to it
There was a problem hiding this comment.
Thanks for catching this as well- this.subFacets should have been initialized to an empty array, and then we push a new facet onto it on each iteration over the subFacets arg that was passed in
There was a problem hiding this comment.
Gotcha! I was also wondering if it makes sense to create a new Facet instance if the subFacets array contains Facet objects already. I'm thinking about this more too, and I wonder if this is a good use of a class. I know that we use a class for the Facet type in Mut, but this could just be an object:
interface Facet {
category: string;
value: string;
subFacet: Array<Facet>;
}
search-manifest/biome.json
Outdated
| @@ -0,0 +1,31 @@ | |||
| { | |||
There was a problem hiding this comment.
We don't need a biome.json for this directory as it finds the nearest biome.json in the project structure.
I think it would be preferable to use the single top level one so that the configurations are consistent, but I'm not opposed to adding one here if there's a specific need.
There was a problem hiding this comment.
No specific need!
| } | ||
| }; | ||
|
|
||
| export const getDocsetsCollection = async () => { |
There was a problem hiding this comment.
This looks great!! Thank you for doing this
|
|
||
| export const getSearchDb = async () => { | ||
| console.log("Getting Search Db"); | ||
| const uri = ENV_VARS.ATLAS_SEARCH_URI; |
There was a problem hiding this comment.
NIT: I think it might be preferable to use the ENV_VARS.ATLAS_SEARCH_URI and other env variables directly as opposed to re-assigning them to another variable. It makes it a little bit harder to track
| const uri = ENV_VARS.ATLAS_CLUSTER0_URI; | ||
| const dbName = ENV_VARS.SNOOTY_DB_NAME; | ||
|
|
||
| if (snootyDb) { |
There was a problem hiding this comment.
I think clusterZeroClient could be more descriptive for this, as this variable doesn't hold the instance of the snootyDb.
What you could do instead is cache the Db instance ie snootyDb = db(dbName)
There was a problem hiding this comment.
I renamed it to clusterZeroClient
| export const getBranch = (branches: Array<BranchEntry>, branchName: string) => { | ||
| for (const branchObj of branches) { | ||
| if (branchObj.gitBranchName.toLowerCase() === branchName.toLowerCase()) { | ||
| return { ...branchObj }; |
There was a problem hiding this comment.
this can just be return branchObj
| )}` | ||
| ); | ||
| // helper function to find the associated branch | ||
| export const getBranch = (branches: Array<BranchEntry>, branchName: string) => { |
There was a problem hiding this comment.
This could be simplified to (branches, branchName) => branches.find(branch => branch === branchName)
TICKET
DOP-5036
This PR cleans up much of the code in the Search-Manifest integration. This includes:
any's