FTN-58: Search by indicator id #29
Conversation
|
|
||
| let searchService: SearchService; | ||
| try { | ||
| searchService = new SearchService(); |
There was a problem hiding this comment.
If I'm understanding this correctly. This will be a singleton of the SearchService instance on the NextJS server to handle all requests. Rather than create a new instance per request. Is this the intended idea here and does this have any draw backs? Just thinking performance, load or concurrency.
There was a problem hiding this comment.
Not sure, although I did think that performing the initial set-up of the search client on each call would be a repetition of effort. My thinking was if Node is single-threaded (think it might be) then creating a new instance each time doesn't help as it's just repeated work, & if it's multi-threaded then there's nothing to stop different threads running the same function.
| const getEnvironmentVariable = (name: string): string => { | ||
| return ( | ||
| process.env[name] ?? | ||
| (() => { | ||
| throw new Error(`Missing environment variable ${name}`); | ||
| })() | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This might make more sense in its own file, as I expect we'll want to use it from other places.
| : { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| searchByIndicator(indicator: string): Promise<IndicatorSearchResult[]> { | ||
| return Promise.resolve(MOCK_DATA); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
It might be cleaner to implement this as a separate implementation of the IndicatorSearch interface.
| const apiKey = getEnvironmentVariable('DHSC_AI_SEARCH_API_KEY'); | ||
|
|
||
| this.searchClient = new SearchClient<Indicator>( | ||
| `https://${serviceName}.search.windows.net`, |
There was a problem hiding this comment.
I appreciate this was how I did it in the PoC, but I think we're probably better taking the full URL to the search service as an environment variable (e.g. https://<serviceName>.search.windows.net), as it means we're not baking in knowledge of the AI search URL structure to our code.
|
Description
Jira ticket: FTN-58
Include a summary of what this pull request is changing and any relevant background context.
Changes
Validation
Details of any validation you have performed to show that your changes are behaving as expected. For example, screenshots of how the UI looks with your changes, or output of any tests you did manually to demonstrate things were working as expected.