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

Method.ts addContextListener signature was not updated for 1.2 and it overrides types for Desktop Agent #435

Closed
kriswest opened this issue Jul 27, 2021 · 4 comments · Fixed by #462
Assignees
Labels
api FDC3 API Working Group bug Something isn't working
Milestone

Comments

@kriswest
Copy link
Contributor

kriswest commented Jul 27, 2021

Minor Issue

We noticed that Methods.ts hasn't been updated for the revised signature for addContextListener. The Methods.ts exports also override the DesktopAgent exports.

Desktop agent:

Methods.ts:

  • v1.2.0 tag:

    FDC3/src/api/Methods.ts

    Lines 70 to 78 in 4cef6a7

    export function addContextListener(contextTypeOrHandler: string | ContextHandler, handler?: ContextHandler): Listener {
    if (typeof contextTypeOrHandler !== 'function') {
    return throwIfNoGlobal(() =>
    window.fdc3.addContextListener(contextTypeOrHandler as string, handler as ContextHandler)
    );
    } else {
    return throwIfNoGlobal(() => window.fdc3.addContextListener(contextTypeOrHandler as ContextHandler));
    }
    }
  • master:

    FDC3/src/api/Methods.ts

    Lines 70 to 78 in 0f240e8

    export function addContextListener(contextTypeOrHandler: string | ContextHandler, handler?: ContextHandler): Listener {
    if (typeof contextTypeOrHandler !== 'function') {
    return throwIfNoGlobal(() =>
    window.fdc3.addContextListener(contextTypeOrHandler as string, handler as ContextHandler)
    );
    } else {
    return throwIfNoGlobal(() => window.fdc3.addContextListener(contextTypeOrHandler as ContextHandler));
    }
    }

Further, we're finding that if you import FDC3 from the NPM module like so:
import * as fdc3 from "@finos/fdc3";
Then the Methods.ts exports take precedence and if you try and use fdc3.addContextListener(null, listener) you'll get a typescript error (however window.fdc3.addContextListener(null, listener) does pick up the right type.

We may need to reconsider how (or even if) these are exported in the index.ts file. I believe they take precedence as they are exported later than the DesktopAgent with the same function names. Some sort of namespacing maybe needed...

Area of Issue

[ ] App Directory
[x] API
[ ] Context Data
[ ] Intents
[ ] Use Cases
[ ] Other

@kriswest kriswest added bug Something isn't working api FDC3 API Working Group labels Jul 27, 2021
@kriswest
Copy link
Contributor Author

@mattjamieson Any chance you could take a look at/have a think about this one?

@kriswest
Copy link
Contributor Author

Fix should be backported into NPM module for 1.2

@kriswest
Copy link
Contributor Author

@mattjamieson @rikoe
Did you get a chance to think about the export issue (Methods.ts exports override DesktopAgent exports)? I think all that stems from the fact that you'll often import fdc3 like so:
import * as fdc3 from "@finos/fdc3";
which will overwrite the DesktopAgent type assigned to window.fdc3 (which is also in scope as fdc3) assigned here:

FDC3/src/index.ts

Lines 24 to 28 in 4cef6a7

declare global {
interface Window {
fdc3: DesktopAgent;
}
}

It may not matter all that much when all the function signatures are in sync and a container provides the API at window.fdc3 (where Methods.ts references it from), but it did in this case.

The solution may be to move the export of Methods.ts under a namespace, e.g.
export * as es6 from './api/Methods';
that would be a breaking change as it would change how you do a restructured import, I'm not sure exactly what to however, perhaps:

import { es6 } from '@finos/fdc3'
const { getOrCreateChannel } = es6;

@mattjamieson
Copy link
Contributor

Hey @kriswest, yes I've been playing with a couple of options. Should get the PR updated tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api FDC3 API Working Group bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants