Bi-directional event handling #285

Abhishk123 · 2025-03-19T12:57:51Z

Description

Enter a detailed description of the PR here, including the nature and purpose of the changes.

JIRA Ticket

Include a link to the JIRA Ticket linked to this PR.
Issue-0000

Additional Info (Test steps / Steps to reproduce etc.)

Details on how to test the feature

github-actions · 2025-03-19T12:59:12Z

Issue with linting or unit tests detected.
Unit tests failed.

github-actions · 2025-03-20T09:26:33Z

Issue with linting or unit tests detected.
Unit tests failed.

Updating log format

# [1.5.0-dev.6](rdkcentral/firebolt-certification-suite@v1.5.0-dev.5...v1.5.0-dev.6) (2024-12-09) ### Bug Fixes * Trimming the new line in logs ([#285](rdkcentral/firebolt-certification-suite#285)) ([36f7b2f](rdkcentral/firebolt-certification-suite@36f7b2f))

jbigel · 2025-05-20T20:42:38Z

test/unit/EventInvocation.test.js

  };
 });

+// jest.mock('../../node_modules/@firebolt-js/sdk/dist/lib/Gateway/index.mjs', () => {


Is this a valid test? Why is it commented?

uncommented and corrected it

jbigel · 2025-05-20T20:42:48Z

test/unit/EventInvocation.test.js

      callId++;
      const id = callId; // a mock ID value
      eventList.push(eventName + '-' + id);
+      console.log('eventList----:', eventList);


is this a debug log?

removed logs

jbigel · 2025-05-20T20:42:59Z

src/EventInvocation.js

+
+  async northBoundEventHandling(message) {
+    try {
+      console.log('----------------------');


is this a debug log?

removed logs

jbigel · 2025-05-20T20:43:13Z

test/unit/EventInvocation.test.js

-      expect(result.id).toBe(expectedResponse.id);
-      expect(result.result).toStrictEqual(expectedResponse.result);
-    });
+    // Check on how to mock the Gateway from firebolt v2 and use it in the test


same as above

uncommented and corrected it

ibrahimshuail · 2025-05-21T15:03:55Z

src/App.js

      this.accessibilityCheck(voiceAnnouncement);
    });
    this.toastStates = [];
+    // Setting the default execution to the Firebolt v2


This needs to be documented somewhere because the end user will not check the code

Added comments

ibrahimshuail · 2025-05-21T15:08:38Z

src/EventInvocation.js

+      return [err, null];
+    }
+
+    // Construct unique key for event handler. A UUID can be added to the key to make it more unique.


can this be more descriptive so we will get know why we are creating this UUID

ibrahimshuail · 2025-05-21T15:11:32Z

src/EventInvocation.js

        });
+        eventHandlerMap.clear();
+        logger.info('After clearing listeners' + JSON.stringify(eventHandlerMap), 'clearAllListeners');
+        return 'Cleared Listeners';


Can we move all these return statements to constants ?

rdkcmf-jenkins · 2025-06-03T09:21:31Z

Coverity detected 1 issue; a quality concern.

src/EventInvocation.js

kschrief · 2025-06-06T17:05:31Z

src/FireboltTransportInvoker.js

        return await invokeProvider.send(module, method, jsonParams);
      } else if (invoker == CONSTANTS.INVOKEMANAGER) {
        return await invokeManager.send(module, method, jsonParams);
+      } else if (process.env.IS_BIDIRECTIONAL_SDK === true || process.env.IS_BIDIRECTIONAL_SDK === 'true') {


What if it comes as "True"?

We can, if we detect a string, run .toLowerCase() and compare that.

if (process.env.IS_BIDIRECTIONAL_SDK === true || (typeof process.env.IS_BIDIRECTIONAL_SDK === 'string' && process.env.IS_BIDIRECTIONAL_SDK.toLowerCase() === 'true')

it wont come as True,
Two cases here, boolean true and string 'true'

Boolean true is assigned in app.js, it will check for sdk version fetching from openrpc and do a regex check of whether it is above 2.x.x and if yes, assign it with true, else false.

String true, the reason string true check is add is because of unit testcases, in jest unit testcases any value that gets assigned to an env will convert to string, whether it is boolean or object or array. So while doing comparision, those unit testcases are failing because of this condition, where true(boolean) is coming as 'true' string.
Value that I am assigning every where in unit testcases is boolean, but still internally it is converting it as a string.
That is the reason, added a check on string true

it won't come as true

We shouldn't assume that. You're checking a process variable that anyone anywhere in FCA can modify. Just because today no one's setting it to "True" or even "tRuE" doesn't mean it won't happen tomorrow.

corrected it

test/jest.transport-resolver.js

src/EventInvocation.js

kschrief · 2025-06-06T17:39:43Z

src/EventInvocation.js

+class EventRegistrationInterface {
+  async clearEventListeners(event) {
+    try {
+      const [sdkType, module] = this.getSdkTypeAndModule(event);


I see us parsing an event over and over in this code.

Can we build a utility function in this file that takes in an event and gives an object containing:

SDK (Core, manage, discovery, etc)

Event Module

Event Method (Ex: onClosedCaptionChanged)

Event Name (Ex: closedCaptionChanged)

Anything else we need

That object can be used where we need it to be.

src/EventInvocation.js

kschrief · 2025-06-11T19:52:16Z

src/EventInvocation.js

+   * parseEventNameAndModuleAndSDKType('firebolt_foo.onExampleEvent')
+   * returns ['firebolt', 'foo', 'onExampleEvent', 'exampleEvent']
+   */
+  parseEventNameAndModuleAndSDKType(moduleWithEventName) {


This is great. Can we use this in the other locations where we're taking a "moduleWithEventName" and parsing it?

corrected it, in most of the places

Added v2 changes

ae7c70e

Addressed the comments

59bf7c5

fixed unit testcases

45d2bae

PreethiMaai pushed a commit that referenced this pull request Apr 10, 2025

fix: Trimming the new line in logs (#285)

36f7b2f

Updating log format

Eswar2103 added 2 commits May 15, 2025 16:51

Merge branch 'dev' into biDirectionalEventHandlin

554cb71

Firebolt v2 event changes

8309f81

Eswar2103 changed the base branch from dev to FIREBOLT-V2 May 20, 2025 15:11

jbigel reviewed May 20, 2025

View reviewed changes

ibrahimshuail reviewed May 21, 2025

View reviewed changes

Eswar2103 added 3 commits May 31, 2025 15:32

V1 event changes

06a0541

Updated import of Transport for v1 and v2

022acf3

Alias correction

b52c336

rdkcmf-jenkins reviewed Jun 3, 2025

View reviewed changes

src/EventInvocation.js Outdated Show resolved Hide resolved

Eswar2103 added 5 commits June 3, 2025 16:34

Code Optimisation

f0ac7c9

Removed console logs

87dd097

Added comments

16e1f20

Fixed unit testcases

c05ec38

Added comments for unit testcases

984bee2

kschrief requested changes Jun 6, 2025

View reviewed changes

Eswar2103 added 2 commits June 9, 2025 12:50

Changed function names and removed un-necessary maps

11a32dc

Added awaits for async functions

50552d4

kschrief requested changes Jun 11, 2025

View reviewed changes

improved condition check

3bc50f3

Added null checks

10dcea2

kschrief approved these changes Jun 13, 2025

View reviewed changes

kschrief approved these changes Jun 16, 2025

View reviewed changes

jbigel approved these changes Jun 16, 2025

View reviewed changes

Resolve merge conflicts

ff1bf95

Eswar2103 merged commit 74ef8c6 into FIREBOLT-V2 Jun 23, 2025

Eswar2103 deleted the biDirectionalEventHandlin branch June 23, 2025 09:05

Bi-directional event handling #285

Bi-directional event handling #285

Uh oh!

Conversation

Abhishk123 commented Mar 19, 2025

Description

JIRA Ticket

Additional Info (Test steps / Steps to reproduce etc.)

Uh oh!

github-actions bot commented Mar 19, 2025

Uh oh!

github-actions bot commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdkcmf-jenkins commented Jun 3, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eswar2103 Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eswar2103 Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Eswar2103 Jun 9, 2025 •

edited

Loading

Eswar2103 Jun 12, 2025 •

edited

Loading