-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(javascript): expose requestOptions
and cache options
#283
Changes from all commits
be0aef9
4400bd6
adb9896
7aeba74
46cbe6a
bebc94c
bf5a304
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
5 | ||
6 |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -7,6 +7,8 @@ import type { | |||||||
StackFrame, | ||||||||
TransporterOptions, | ||||||||
Transporter, | ||||||||
Headers, | ||||||||
QueryParameters, | ||||||||
} from '../types'; | ||||||||
|
||||||||
import { createStatefulHost } from './createStatefulHost'; | ||||||||
|
@@ -217,9 +219,37 @@ export function createTransporter({ | |||||||
} | ||||||||
|
||||||||
function createRequest<TResponse>( | ||||||||
request: Request, | ||||||||
requestOptions: RequestOptions | ||||||||
baseRequest: Request, | ||||||||
methodOptions: { | ||||||||
headers: Headers; | ||||||||
queryParameters: QueryParameters; | ||||||||
}, | ||||||||
baseRequestOptions?: RequestOptions | ||||||||
): Promise<TResponse> { | ||||||||
const mergedData: Request['data'] = Array.isArray(baseRequest.data) | ||||||||
? baseRequest.data | ||||||||
: { | ||||||||
...baseRequest.data, | ||||||||
...baseRequestOptions?.data, | ||||||||
}; | ||||||||
const request: Request = { | ||||||||
...baseRequest, | ||||||||
data: mergedData, | ||||||||
}; | ||||||||
const requestOptions: RequestOptions = { | ||||||||
cacheable: baseRequestOptions?.cacheable, | ||||||||
timeout: baseRequestOptions?.timeout, | ||||||||
Comment on lines
+240
to
+241
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's indeed better but small size impact, so I've went with that instead |
||||||||
queryParameters: { | ||||||||
...baseRequestOptions?.queryParameters, | ||||||||
...methodOptions.queryParameters, | ||||||||
}, | ||||||||
headers: { | ||||||||
Accept: 'application/json', | ||||||||
...baseRequestOptions?.headers, | ||||||||
...methodOptions.headers, | ||||||||
}, | ||||||||
}; | ||||||||
Comment on lines
+229
to
+251
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of this part, but those changes at the method level added like 1kb to the bundle size of each clients There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Logic could definitely be extracted in a method) |
||||||||
|
||||||||
if (request.method !== 'GET') { | ||||||||
/** | ||||||||
* On write requests, no cache mechanisms are applied, and we | ||||||||
|
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.
Is it really useful to have optional data ? Do you have a use case in mind ?
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.
The only thing I could see is when an option is not available in the client but exists on tHe REST api.
e.g. if they try dev things on the engine, they can test it without changing clients.
It was already there in v4 so I've kept it
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.
Okay this is already handled by the
customRequest
endpoint but why not