Skip to content
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: support trigger events option #745

Closed
wants to merge 13 commits into from

Conversation

Juiced66
Copy link
Contributor

@Juiced66 Juiced66 commented Sep 17, 2024

This PR is intended to support triggerEvents request parameter for embeded sdk.

It is unit tested and will be functionnal tested when released on kuzzle side.

A lot of doc as been realigned with code

JIRA: KZLPRD-579

@Juiced66 Juiced66 self-assigned this Sep 17, 2024
@Juiced66 Juiced66 force-pushed the feat/support-triggerEvents-option branch from 1285dee to a146d15 Compare September 17, 2024 13:50
@Juiced66 Juiced66 marked this pull request as draft September 17, 2024 13:50
@Juiced66 Juiced66 changed the title Feat/support trigger events option feat: support trigger events option Sep 17, 2024
@Juiced66 Juiced66 force-pushed the feat/support-triggerEvents-option branch from 09c372f to 5e5f12a Compare September 20, 2024 09:03
@Juiced66 Juiced66 force-pushed the feat/support-triggerEvents-option branch from e907957 to 999f1a1 Compare September 23, 2024 13:00
@Juiced66 Juiced66 marked this pull request as ready for review September 24, 2024 08:04
Copy link

@afondard afondard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@Kuruyia
Copy link

Kuruyia commented Oct 1, 2024

Why didn't you put this (duplicated) piece of code in the query method directly?

if (options.triggerEvents !== undefined) {
  request.triggerEvents = options.triggerEvents;
}

I see that stuff like this is already done in this method:

if (
request.retryOnConflict === undefined &&
options.retryOnConflict !== undefined
) {
request.retryOnConflict = options.retryOnConflict;
}

Copy link
Contributor

@rolljee rolljee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything look good and updated.

There is just something odd about the :any you added to the request.
Since you have the type for the KuzzleRequest isn't that possible to put the type instead of any ?

Or like Alex said, it could be possible to avoid duplication at the Kuzzle.ts file level

@Juiced66
Copy link
Contributor Author

Juiced66 commented Oct 1, 2024

Why didn't you put this (duplicated) piece of code in the query method directly?

if (options.triggerEvents !== undefined) {
  request.triggerEvents = options.triggerEvents;
}

I see that stuff like this is already done in this method:

if (
request.retryOnConflict === undefined &&
options.retryOnConflict !== undefined
) {
request.retryOnConflict = options.retryOnConflict;
}

I've thought about that, it was an option but not all methods got options and it could lead to useless code for those. I thought it was less readable to apply it were needed but more optimized. If we decide to apply it directly on query it is doable for sure

@Juiced66
Copy link
Contributor Author

Juiced66 commented Oct 1, 2024

Everything look good and updated.

There is just something odd about the :any you added to the request. Since you have the type for the KuzzleRequest isn't that possible to put the type instead of any ?

Or like Alex said, it could be possible to avoid duplication at the Kuzzle.ts file level

There's no KuzzleRequest type in js SDK
I could use Omit<RequestPayload, "controller"> wiches weird too but maybe more acceptable. Controller is abstracted and taken from controller name and it leads to ts errors without omitting

src/controllers/Bulk.ts Outdated Show resolved Hide resolved
@Juiced66
Copy link
Contributor Author

Juiced66 commented Oct 1, 2024

For maintainability purpose and after doing pro and cons, I'll stick to maintainability purpose where the query solution is better than little optim nit picking.

I will refactor it

@rolljee
Copy link
Contributor

rolljee commented Oct 9, 2024

LIttle UP as this need to be merged 💪🏼

@Juiced66
Copy link
Contributor Author

will close this to open a Doc only PR

@Juiced66 Juiced66 closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants