[Client] adding nodejs cac and experimentation client wrapper #170

namitgoel · 2024-07-15T14:53:29Z

Problem

Describe the problem you are trying to solve here

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint	Method	Request body	Response Body
API	GET/POST, etc	request	response

Possible Issues in the future

Describe any possible issues that could occur because of this change

Datron · 2024-07-24T06:00:02Z

@namitgoel Can you add a README file that documents how to add the nodeJS clients to a project, and also how to build and run them?

Datron · 2024-07-24T06:00:15Z

clients/nodejs/cac-client/client.ts

+
+let file =
+    platform == "darwin" ?
+    '../../../target/debug/libcac_client.dylib' :


@namitgoel lets avoid relative paths

Datron

Documentation needed + relative paths pointing to the binaries is an issue

Datron · 2024-07-24T06:02:07Z

clients/nodejs/cac-client/client.ts

+        this.cacHostName = cacHostName;
+    }
+
+    public getCACLastErrorMessage(): string | null {


Can we return just string?

@Datron we cannot just return string as the response type that ref-napi libraray is providing has return type string | null

Datron · 2024-07-24T06:02:19Z

clients/nodejs/cac-client/client.ts

+        this.rustLib.cac_start_polling_update(this.tenant)
+    }
+
+    public getCACConfig(filterQuery: string, filterPrefix: string): string | null {


Suggested change

public getCACConfig(filterQuery: string, filterPrefix: string): string | null {

public getCACConfig(filterQuery: string, filterPrefix: string): string | Error {

ShubhranshuSanjeev · 2024-07-24T06:24:58Z

clients/nodejs/cac-client/client.ts

+
+let file =
+    platform == "darwin" ?
+    '../../../target/debug/libcac_client.dylib' :


Are we sure that file will be present at this path in case of release build too (its specifically pointing to debug directory)?

ShubhranshuSanjeev · 2024-07-24T06:27:39Z

clients/nodejs/cac-client/client.ts

+        return this.rustLib.cac_get_last_modified(this.getCACClient());
+    }
+
+    public getResolvedConfig(query: string, filterKeys: string, mergeStrategy: string): string | null {


Why return type for all functions is string, I am assuming its a JSON string?
Can we define types for these and de-serialize and send an actual object back?

ayushjain17

most comments apply to both experimentation and cac file, have not repeated few comments

ayushjain17 · 2024-08-26T10:09:21Z

clients/nodejs/cac-client/client.ts

+    });
+
+    constructor(tenantName: string, pollingFrequency: number, cacHostName: string) {
+        if (tenantName == "" || cacHostName == "") {


lets separate the if checks, its returning tenantName cannot be null or empty error when either tenantName or cacHostName is empty

ayushjain17 · 2024-08-26T10:13:46Z

clients/nodejs/cac-client/client.ts

+        return this.rustLib.cac_get_last_modified(this.getCACClient());
+    }
+
+    public getResolvedConfig(query: Object, filterKeys: string[] | null, mergeStrategy: string): string | null {


mergeStrategy is an enum: MERGE, REPLACE

ayushjain17 · 2024-08-26T10:18:50Z

clients/nodejs/cac-client/client.ts

+        return this.rustLib.cac_last_error_message();
+    }
+
+    public getCACLastErrorLength(): number {


can we avoid repeating CAC in every function name, these functions are all scoped to the object of CacClient, so adding CAC again in the function name, makes it somewhat redundant

ayushjain17 · 2024-08-26T10:22:27Z

clients/nodejs/cac-client/client.ts

+        return this.rustLib.cac_get_client(this.tenant);
+    }
+
+    public createNewCACClient(): number {


the logic for this should ideally be inside the constructor itself, in haskell we needed a separate function, because that's that does not have any constructor, here since there is a constructor, this logic should be inside that itself

ayushjain17 · 2024-08-26T12:42:50Z

clients/nodejs/exp-client/client.ts

+        return this.rustLib.expt_last_error_message();
+    }
+
+    public createNewExperimentaionClient(): number {


same as CacClient

ayushjain17 · 2024-08-26T12:55:24Z

clients/nodejs/cac-client/client.ts

+
+    public getResolvedConfig(query: Object, filterKeys: string[] | null, mergeStrategy: string): string | null {
+        let strQuery = JSON.stringify(query);
+        let strFilterKeys = filterKeys == null ? ref.NULL : ref.allocCString(filterKeys.join(this.delimeter));


currently the delimiter being used for this function is "|"

ayushjain17 · 2024-08-26T12:57:51Z

clients/nodejs/exp-client/client.ts

+        return this.rustLib.expt_get_running_experiments(clientPtr);
+    }
+
+    public freeString(str: string) {


this is not needed as an exposed function

ayushjain17 · 2024-08-26T13:12:16Z

clients/nodejs/exp-client/client.ts

+        return this.rustLib.expt_last_error_length()
+    }
+
+    public freeExprementationClient() {


this is not needed as an exposed function

ayushjain17 · 2024-08-26T13:27:29Z

clients/nodejs/cac-client/client.ts

+            this.getCACClient(), strQuery, strFilterKeys, mergeStrategy
+        )
+    }
+    public getDefaultConfig(filterKeys: string[] | null): string | null {


if the output of the functions are null, then you should check for the error and then return the error caused during the function call (for all functions returning null | some_value)

ayushjain17 · 2024-08-26T13:30:21Z

clients/nodejs/exp-client/client.ts

+        this.rustLib.expt_free_client(this.getExperimentationClient());
+    }
+
+    public getFilteredSatisfiedExperiments(context: Object, filterPrefix: string[] | null): string | null {


places which take input of the form: some_type | null, should ideally take some_type | undefined type data

ayushjain17 · 2024-08-27T07:42:23Z

Also, make sure to deallocate the data allocated via after it has been used

namitgoel requested a review from a team as a code owner July 15, 2024 14:53

namitgoel force-pushed the nodejs-cac-client branch from a4de85d to 02bdbff Compare July 19, 2024 12:19

Datron reviewed Jul 24, 2024

View reviewed changes

Datron requested changes Jul 24, 2024

View reviewed changes

Datron reviewed Jul 24, 2024

View reviewed changes

ShubhranshuSanjeev requested changes Jul 24, 2024

View reviewed changes

Datron requested review from Datron and ShubhranshuSanjeev July 30, 2024 06:48

namitgoel force-pushed the nodejs-cac-client branch 2 times, most recently from e14669d to 5364c56 Compare August 4, 2024 14:01

namitgoel force-pushed the nodejs-cac-client branch 2 times, most recently from 8e1f007 to 382aba6 Compare August 25, 2024 15:46

ayushjain17 requested changes Aug 26, 2024

View reviewed changes

feat: adding nodejs cac and experimentation client wrapper

a52defd

namitgoel force-pushed the nodejs-cac-client branch from 382aba6 to a52defd Compare August 29, 2024 08:37

namitgoel requested a review from ayushjain17 August 29, 2024 15:45

Datron force-pushed the main branch 10 times, most recently from ae783fd to a9b01a6 Compare September 6, 2024 15:22

Datron force-pushed the main branch from 02832c3 to a9afdb9 Compare September 9, 2024 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Client] adding nodejs cac and experimentation client wrapper #170

[Client] adding nodejs cac and experimentation client wrapper #170

namitgoel commented Jul 15, 2024

Datron commented Jul 24, 2024

Datron Jul 24, 2024

namitgoel Jul 29, 2024

Datron left a comment

Datron Jul 24, 2024

namitgoel Jul 26, 2024

Datron Jul 24, 2024

ShubhranshuSanjeev Jul 24, 2024

ShubhranshuSanjeev Jul 24, 2024

ayushjain17 left a comment

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

ayushjain17 Aug 26, 2024

ayushjain17 Aug 26, 2024

namitgoel Aug 29, 2024

ayushjain17 Aug 26, 2024

ayushjain17 commented Aug 27, 2024

	public getCACConfig(filterQuery: string, filterPrefix: string): string \| null {
	public getCACConfig(filterQuery: string, filterPrefix: string): string \| Error {

[Client] adding nodejs cac and experimentation client wrapper #170

Are you sure you want to change the base?

[Client] adding nodejs cac and experimentation client wrapper #170

Conversation

namitgoel commented Jul 15, 2024

Problem

Solution

Environment variable changes

Pre-deployment activity

Post-deployment activity

API changes

Possible Issues in the future

Datron commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushjain17 commented Aug 27, 2024