[NO-REF] - introduce pia http module#449
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new Product Insights Agent (PIA) HTTP module to the ConstructorIO client, including TypeScript typings and mocha specs, and exposes it as constructorio.pia.
Changes:
- Added
src/modules/pia.jswithgetSuggestedQuestionsandgetAnswerResultsAPI methods. - Exposed the new module via
src/constructorio.jsand updated TypeScript declarations to includepia. - Added mocha specs for the new PIA module.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/pia.d.ts | Adds TypeScript declarations for the new Pia module and its request/response types. |
| src/types/index.d.ts | Re-exports Pia types from the types entrypoint. |
| src/types/constructorio.d.ts | Adds pia: Pia to the ConstructorIO type surface and namespace exports. |
| src/modules/pia.js | Implements the Pia HTTP module (URL construction + fetch calls). |
| src/constructorio.js | Instantiates and exposes this.pia on the main client. |
| spec/src/modules/pia.js | Adds mocha coverage for suggested questions + answer results, including timeout cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { | ||
| apiKey, | ||
| clientId, | ||
| sessionId, | ||
| segments, | ||
| userId, | ||
| version, | ||
| agentServiceUrl, | ||
| } = options; |
| const controller = new AbortController(); | ||
| const { signal } = controller; | ||
|
|
||
| try { | ||
| requestUrl = createPiaUrl(itemId, parameters, this.options, ''); | ||
| } catch (e) { | ||
| return Promise.reject(e); | ||
| } | ||
|
|
||
| helpers.applyNetworkTimeout(this.options, networkParameters, controller); | ||
|
|
||
| return fetch(requestUrl, { signal }) |
| getAnswerResults(itemId, question, parameters, networkParameters = {}) { | ||
| let requestUrl; | ||
| const { fetch } = this.options; | ||
| const controller = new AbortController(); | ||
| const { signal } = controller; | ||
|
|
…rio-client-javascript into NO-REF/introduce-pia-http-module
esezen
left a comment
esezen
left a comment
There was a problem hiding this comment.
Thanks for working on this, I left some small comments.
| if (!question || typeof question !== 'string') { | ||
| return Promise.reject(new Error('question is a required parameter of type string')); | ||
| } |
There was a problem hiding this comment.
Can we move this up to come before AbortController so that we never even instantiate the AbortController if the question is not valid?
| return json; | ||
| } | ||
|
|
||
| throw new Error('getSuggestedQuestions response data is malformed'); |
There was a problem hiding this comment.
Can we add a test to check for this case?
| } | ||
|
|
||
| if (parameters) { | ||
| const { threadId, variationId, numResults } = parameters; |
There was a problem hiding this comment.
Can we update the tests to check for threadId?
| queryParams.variation_id = variationId; | ||
| } | ||
|
|
||
| if (!helpers.isNil(numResults)) { |
There was a problem hiding this comment.
Same as above, can we test this in one of the tests?
| } | ||
|
|
||
| export interface PiaAnswerItemResults { | ||
| request?: Record<string, any>; |
There was a problem hiding this comment.
Is this actually very loose or do we know the shape of this? If we do, can we type it strictly? I know we have Record<string, any> in other parts of the library but we are trying to move away from that
No description provided.