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

Context Clearing #1379

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

kemerava
Copy link
Contributor

Closes #1197

Adding context cleaning

@michael-bowen-sc

@kemerava kemerava requested a review from a team as a code owner September 27, 2024 18:37
Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit a2c348e
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/66ffeb2da151d20008e46459
😎 Deploy Preview https://deploy-preview-1379--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -36,6 +36,7 @@ interface Channel {
broadcast(context: Context): Promise<void>;
getCurrentContext(contextType?: string): Promise<Context|null>;
addContextListener(contextType: string | null, handler: ContextHandler): Promise<Listener>;
clearContext(contextType?: string): Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should require an explicit null argument (contextType: string | null) to clear all types - which would match addContectListener or stick with this (matching getCurrentContext). Either works of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me it makes sense to keep it closer to getCurrentContext, because they seem most related to me, but happy to hear any thoughts/suggestions.

Copy link
Contributor

@kriswest kriswest Oct 8, 2024

Choose a reason for hiding this comment

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

There is a historical reason for addContextListener having an explicit null argument, the contextType argument was originally optional so the handler argument could be either first or second (when a type was specified). This was NOT good in practice so we moved to an explicit null argument and deprecated the single argument override:
https://fdc3.finos.org/docs/2.0/api/ref/DesktopAgent#addcontextlistener-deprecated

We don't have that problem here, so I agree its fine as is (matching getCurrentContext).

@@ -57,6 +58,7 @@ interface IChannel: IIntentResult
Task Broadcast(IContext context);
Task<IContext?> GetCurrentContext(string? contextType);
Task<IListener> AddContextListener<T>(string? contextType, ContextHandler<T> handler) where T : IContext;
Task ClearContext(string? contextType);
Copy link
Contributor

Choose a reason for hiding this comment

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

docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

Many thanks for taking this on @kemerava.

Could you add an entry to the CHANGLOG.md file please.

I think we should keep this open until after fdc3 for the web (#1191) is merged, when we will need to add schemas for a request and response message. The same may be needed in the bridging (the fdc3.nothing ewill propagate through normal broadcast messages, but I think this will need a specific message to indicate it happened through the clearContextAPI call).

docs/api/spec.md Outdated Show resolved Hide resolved
schemas/context/nothing.schema.json Outdated Show resolved Hide resolved
schemas/context/nothing.schema.json Outdated Show resolved Hide resolved
src/api/Channel.ts Outdated Show resolved Hide resolved
src/api/Channel.ts Outdated Show resolved Hide resolved
src/api/Channel.ts Outdated Show resolved Hide resolved
@kriswest kriswest added this to the 2.3 candidates milestone Oct 1, 2024
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM - although we could do with an example of listening for cleared context somewhere.

Holding approval until after fdc3-for-web is merged as we'll then need to add schemas to support it there. Bridging schemas are also needed, those should be added before this is merged.

Many thanks for the contribution @kemerava, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context Clearing
2 participants