-
Notifications
You must be signed in to change notification settings - Fork 9
INTER-1488: Migrate SDK to APIv4 #217
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
base: main
Are you sure you want to change the base?
Conversation
- Switch base URLs to APIv4 (`https://{region}.api.fpjs.io/v4`) and add
`apiVersion` handling in URL builder.
- Use `Authorization: Bearer <secret-api-key>` only. Remove custom
header and query api authorization.
- Event API renamed and updated. Use `event_id` instead of `request_id`.
Also use `PATCH` for event update instead of `PUT` method.
- Add new examples, mocked responses, and update `sync.sh` script.
- README reworked for v4
- Package renamed to `@fingerprint/fingerprint-server-sdk`. Updated
description and subpackages (example).
- Regenerated types and OpenAPI schema.
- Updated unit and mock tests for v4 URLs.
BREAKING CHANGES:
- Only **Bearer auth** is supported; query and custom-header API-key
modes are removed. `AuthenticationMode` option is deleted.
- Event endpoint and signatures changes:
- Use `client.getEvent(eventId)` instead of `requestId`
Related-Task: INTER-1488
🦋 Changeset detectedLatest commit: bbf565b The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Remove `Accept: application/json` testing header for `updateEvent` function. Related-Task: INTER-1488
Fix updateEventTests test expected method to `PATCH`. Related-Task: INTER-1488
Coverage report
Show files with reduced coverage 🔻
Test suite run success56 tests passing in 9 suites. Report generated by 🧪jest coverage report action from bbf565b Show full coverage report
|
Removed unnecessary commented line. Related-Task: INTER-1488
Expands `searchEvents` JSDoc with new filters. Also its align parameter names.
|
Sharing feedback (more to come later):
I will try to wrap up my review ASAP, and share more if there is more. Thank you! @erayaydin |
src/serverApiClient.ts
Outdated
|
|
||
| const headers = this.getHeaders() | ||
|
|
||
| const response = await this.fetch(url, { |
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.
[nitpick]: We use getRequestPath to calculate the url, but we don't reuse all the options we provide to getRequestPath when making this fetch all. One example is the method param, we have to provide method: 'delete' twice, once in getRequestPath and once in fetch. This is not a healthy design. We can consider doing something like below:
function callApi(reqParams: ReqParams, headers: Headers) {
const url = getRequestPath({
path: reqParams.path,
region: reqParams.region,
pathParams: reqParams.pathParams,
method: reqParams.method,
})
return this.fetch(url, {method: options.method, headers: headers})
}
The point is to make sure getRequestPath and this.fetch use the same params.
This can be treated as tech debt for later, up to you.
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.
I'm curious, why do we even need method in getRequestPath?
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.
From the codebase:
// method mention here so that it can be referenced in JSDoc
// eslint-disable-next-line @typescript-eslint/no-unused-vars
@TheUnderScorer could you please give more context about this issue?
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.
Updated the structure of usage, but it still uses method. Could you please review the new structure @necipallef
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.
Looks good to me! Feel free to resolve the conversation if no more input needed from anybody
| * | ||
| * @param body - Data to update the event with. | ||
| * @param requestId The unique event [identifier](https://dev.fingerprint.com/docs/js-agent#requestid). | ||
| * @param eventId The unique event [identifier](https://dev.fingerprint.com/docs/js-agent#eventid). |
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 link anchor is not found
| export type EventsGetResponse = paths['/events/{event_id}']['get']['responses']['200']['content']['application/json'] | ||
|
|
||
| /** | ||
| * More info: https://dev.fingerprintjs.com/docs/webhooks#identification-webhook-object-format |
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.
Incorrect anchor. It should be https://dev.fingerprint.com/docs/webhooks#webhook-payload-format instead.
| } | ||
| } | ||
| updateEvent: { | ||
| parameters: { | ||
| query?: never | ||
| header?: never | ||
| path: { | ||
| /** @description The unique event [identifier](https://dev.fingerprint.com/reference/get-function#requestid). */ | ||
| request_id: string | ||
| /** @description The unique event [identifier](https://dev.fingerprint.com/reference/get-function#event_id). */ |
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.
invalid link anchor
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.
left comments, some are nitpicks, some are change needed requests
Removes `example/getVisitorHistory.mjs` file because it's not a different endpoint anymore. Related-Task: INTER-1488
Add `fingerprint-server-sdk-smoke-tests` to changeset ignore list. Related-Task: INTER-1488
Explicitly mention about package name change in changeset file. Related-Task: INTER-1488
Use correct `patch` method instead of `put` for updating an event. Related-Task: INTER-1488
Replace strict `response.status === 200` checks with `response.ok` in fetch handlers. Related-Task: INTER-1488
Added a new `callApi()` function to handle request building and `fetch` usage in one place. Replaced all direct `fetch` calls with `callApi`. Introduced `defaultHeaders` options to the client, it's include `Authorization` header and allow extra headers and override of `Authorization` header. Made `region` optional in `GetRequestPathOptions` and it's default to `Region.Global` in `getRequestPath` function. Related-Task: INTER-1488
Remove `isEmptyValue` helper function and use direct `== null` checks to skip `undefined` or `null` values. Related-Task: INTER-1488
Use correct `EVENT_ID` placeholder for the example `.env.example` dotenv file. Related-Task: INTER-1488
Add `IsNever` and `NonNeverKeys` utility types to filter out `never` type keys. Introduce and export `AllowedMethod` that excludes specific `parameters` and any `never` type methods. Use this `AllowedMethod` type for `GetRequestPathOptions` type and `getRequestPath` functions. Update related signatures. Related-Task: INTER-1488
Use `options.method.toUpperCase()` when calling `fetch` to ensure standard HTTP method casing and avoid test fails. Change `AllowedMethod` to make a string literal union. Related-Task: INTER-1488
Extend `callApi` to accept an optional `body?: BodyInit` and forward it to `fetch`. Fix `updateEvent` to send `body`. Related-Task: INTER-1488
Removed unnecessary visitor detail tests. Related-Task: INTER-1488
Fix missing quote for `linked_id` in `searchEvents.mjs` example file. Related-Task: INTER-1488
|
Can we also update this line from |
Removed redundant `await` keyword in `callApi` function. Related-Task: INTER-1488
|
|
||
| #### Webhook types | ||
|
|
||
| When handling [Webhooks](https://dev.fingerprint.com/docs/webhooks) coming from Fingerprint, you can cast the payload as the built-in `VisitWebhook` type: |
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.
| When handling [Webhooks](https://dev.fingerprint.com/docs/webhooks) coming from Fingerprint, you can cast the payload as the built-in `VisitWebhook` type: | |
| When handling [Webhooks](https://dev.fingerprint.com/reference/posteventwebhook#/) coming from Fingerprint, you can cast the payload as the built-in `Event` type: |
Not sure about the link, but it should be more relevant.
| import { Event } from '@fingerprint/fingerprint-server-sdk' | ||
|
|
||
| const visit = visitWebhookBody as unknown as VisitWebhook | ||
| const visit = visitWebhookBody as unknown as Event |
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.
| const visit = visitWebhookBody as unknown as Event | |
| const visit = eventWebhookBody as unknown as Event |
Centralize response parsing and error handling in `callApi` function. Throw consistent `SdkError`, `RequestError`, or `TooManyRequestsError` depends on the case. Added `SuccessJsonOrVoid<Path, Method>` to automatically resolve the potential/correct return type for success responses. Simplify all public methods. Related-Task: INTER-1488
Removed `handleErrorResponse` and moved all error logic into `callApi`. Exported `isErrorResponse` to detect valid error payloads. Added `createResponse` helper for creating mock Response object with headers. Related-Task: INTER-1488
🚀 Following releases will be created using changesets from this PR:@fingerprint/[email protected]Major Changes
|
| return jsonResponse as RelatedVisitorsResponse | ||
| let errPayload | ||
| try { | ||
| // TODO: Use ErrorJson<Path, Method> instead of ErrorResponse type. It requires generic error classes without error.message and error.code |
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 intentional to keep this Todo item? Also, the ErrorJson type is not used anywhere
This PR upgrades the Server SDK to the Server API v4. It removes Server APIv3 specific behavior and aligns the SDK with the new events format shared by Server API and Webhooks.
❓ Why?
data/products/resultnesting), switches to Bearer auth, unifies Webhooks & Server APIeventformats, and deprecates/visitorsin favor of/v4/events.⚙️ What Changed?
GET /events/search->GET /v4/eventsGET /visitors-> removed, migrate toGET /v4/events/v4/*URLs.Auth-Api-Keycustom header andapiKeyquery param withAuthorization: Bearer <secret_api_key>.AuthenticationModeoption.request_id->event_idlinked_id,pagination_key)GET /v4/eventswith filters.PUT /events->PATCH /v4/event/:event_idAuthenticationModeoption are removed.getEvent(requestId)->getEvent(eventId)updateEvent(body, eventId)now use snake_case fields📦 Migration Guide (SDK Consumers)
authenticationModeoption when initializingFingerprintJsServerApiClient.const client = new FingerprintJsServerApiClient({ apiKey: '<SECRET_API_KEY>', region: Region.Global, - authenticationMode: AuthenticationMode.AuthHeader })getEvent()function, useeventIdparameter name instead ofrequestId.client.updateEvent({ - linkedId: "linkedId" + linked_id: "linkedId" }, "<event-id>")tagsinstead oftagfor updating an event.client.updateEvent({ - tag: { + tags: { key: 'value', }, }, "<event-id>")updateEvent()function, useeventIdparameter name instead ofrequestId.client.updateEvent( {}, - requestId: "<request-id>" + eventId: "<event-id>" )getVisitorHistory()function.getVisits()function. UsesearchEvents()function.getRelatedVisitors()function.VisitorHistoryFilter,ErrorPlainResponse,VisitorsResponse,RelatedVisitorsResponse,RelatedVisitorsFilter,Webhook,EventsUpdateRequesttypes.