Dev/tim/noodles #196

tim-dinsdale · 2019-12-19T17:45:35Z

noodly noodly

…g errors on files I did not change

tomrobertsOF · 2020-01-06T13:25:37Z

src/client/connection.ts

-            channelPromise = fin.InterApplicationBus.Channel.connect(SERVICE_CHANNEL, {payload: {version: PACKAGE_VERSION}}).then((channel: ChannelClient) => {
-                // Register service listeners
-                channel.register('WARN', (payload: unknown) => console.warn(payload));  // tslint:disable-line:no-any
+            channelPromise = new Promise<ChannelClient>((resolve, reject) => {


Not sure what we gain from wrapping this in another promise, since the .then will return a promise anyway. You can just make the inner function async and use return/throw instead of resolve/reject.

I know this is more about content than style at the moment, but figure it's worth mentioning.

tomrobertsOF · 2020-01-06T13:28:29Z

src/client/connection.ts

-                channel.register('WARN', (payload: unknown) => console.warn(payload));  // tslint:disable-line:no-any
+            channelPromise = new Promise<ChannelClient>((resolve, reject) => {
+                fin.System.getRuntimeInfo().then((info: RuntimeInfo & {fdc3AppUuid?: string; fdc3ChannelName?: string}) => {
+                    if (info.fdc3AppUuid && info.fdc3ChannelName) {


Does this actually work right now, or are you adding these in anticipation of the runtime doing it in a later build?

tomrobertsOF · 2020-01-06T13:29:29Z

src/client/internal.ts

@@ -15,18 +15,24 @@ import {AppIntent, Context, IntentResolution, Listener} from './main';
 import {ChannelId, DefaultChannel, SystemChannel, DisplayMetadata, ChannelWindowAddedEvent, ChannelWindowRemovedEvent, ChannelChangedEvent, ChannelBase, AppChannel} from './contextChannels';
 import {FDC3Error} from './errors';

+export interface ServiceIdentity{


Why do we need another identity type just for the service? UUID/Name pairs are used all over the place so we should keep it consistent.

If you do keep it for any reason, brace formatting

tomrobertsOF · 2020-01-06T13:31:26Z

src/client/internal.ts

+export function getServiceIdentityUUID(): string {
+    return serviceIdentity.uuid;
+}
+
+export function getServiceIdentityName(): string {
+    return serviceIdentity.name;
+}


These don't really seem necessary to me. How is getServiceIdentityUUID() any better than getServiceIdentity().uuid?

tomrobertsOF · 2020-01-06T13:35:21Z

src/provider/controller/ChannelHandler.ts

@@ -18,9 +20,14 @@ export class ChannelHandler {
    public readonly onChannelChanged: Signal<[AppConnection, ContextChannel | null, ContextChannel | null], Promise<void>>;

    private readonly _model: Model;
+    private readonly _mrh: MultiRuntimeHandler;


This isn't rust; you can afford to write out an actual variable name.

tomrobertsOF · 2020-01-06T14:45:06Z

src/provider/controller/MultiRuntimeHandler.ts

+        this._channelHandler.onChannelChanged.add(this.onChannelChangedHandler, this);
+    }
+
+    private async onChannelChangedHandler(connection: AppConnection, channel: ContextChannel | null, previousChannel: ContextChannel | null): Promise<void> {


on[...]Handler seems a bit redundant. Would make it channelChangeHandler, onChannelChanged, or better yet handleChannelChanged to match the other methods in this class.

tomrobertsOF · 2020-01-06T14:46:49Z

src/provider/controller/MultiRuntimeHandler.ts

+    }
+
+    protected async init(): Promise<void> {
+        const subMsg = (msg: Msg, sourceUUID: string, sourceName: string) => {


Any particular reason to have this assigned? Could it not just be passed directly into the subscribe call?

tomrobertsOF · 2020-01-06T14:50:42Z

src/provider/common/Injectables.ts

@@ -14,6 +14,7 @@ enum Injectable {
    EVENT_HANDLER,
    INTENT_HANDLER,
    MODEL,
+    MULTI_RUNTIME_HANDLER,


You've added the injectable key, but also need to add a type and a binding for it in injector.ts

tomrobertsOF · 2020-01-06T14:55:54Z

src/provider/controller/MultiRuntimeHandler.ts

+type Msg = MsgRequestGetAll | MsgRespondGetAll | MsgSetColorChannel | MsgRegisterAppInChannel | MsgUnregisterAppFromChannel;
+
+@injectable()
+export class MultiRuntimeHandler extends AsyncInit {


I know we're just in POC for now, but when we do it for real this should not have any direct fin dependencies, and should instead use the injected Environment to do what it needs to do.

tomrobertsOF · 2020-01-06T14:59:24Z

src/provider/model/FinEnvironment.ts

@@ -117,13 +117,13 @@ export class FinEnvironment extends AsyncInit implements Environment {
            interface OFManifest {
                shortcut?: {name?: string; icon: string};
                // eslint-disable-next-line camelcase
-                startup_app: {uuid: string; name?: string; icon?: string};
+                startup_app?: {uuid: string; name?: string; icon?: string};


Is this really optional? Why did this need changing?

rypete

@tomrobertsOF marked a few but, what overall what is the reason for the multiple // eslint-disable-line @typescript-eslint/indent? There are a few formatting errors. I'm wondering if a lint will fix both? Ignore me if linting has changed :)

Can we also have a proper title for this PR? It will look nicer in the history when merged

rypete · 2020-01-20T17:08:03Z

package.json

@@ -52,7 +53,7 @@
    "openfin-service-async": "^1.0.1",
    "openfin-service-config": "1.0.2",
    "openfin-service-signal": "^1.0.0",
-    "openfin-service-tooling": "1.0.21",
+    "openfin-service-tooling": "1.0.23",


The latest openfin-service-tooling is version 1.0.25. Can we bring this in as part of this PR (or separate)?

rypete · 2020-01-20T17:13:57Z

src/client/connection.ts

+                    } else {
+                        fin.InterApplicationBus.Channel.connect(getServiceChannel(), {payload: {version: PACKAGE_VERSION}}).then((channel: ChannelClient) => {
+                            // Register service listeners
+                            channel.register('WARN', (payload: unknown) => console.warn(payload));  // tslint:disable-line:no-any


Still need the // tslint:disable-line:no-any?

rypete · 2020-01-20T17:14:53Z

src/client/internal.ts

@@ -15,18 +15,24 @@ import {AppIntent, Context, IntentResolution, Listener} from './main';
 import {ChannelId, DefaultChannel, SystemChannel, DisplayMetadata, ChannelWindowAddedEvent, ChannelWindowRemovedEvent, ChannelChangedEvent, ChannelBase, AppChannel} from './contextChannels';
 import {FDC3Error} from './errors';

+export interface ServiceIdentity{


If you do keep it for any reason, brace formatting

rypete · 2020-01-20T17:17:51Z

src/provider/controller/MultiRuntimeHandler.ts

+    source: Identity;
+}
+
+interface ChannelState{


Brace formatting

rypete · 2020-01-20T17:20:10Z

src/provider/controller/ResolverHandler.ts

@@ -18,7 +18,7 @@ const RESOLVER_URL = (() => {
    }

    // Locate the default resolver HTML page, relative to the location of the provider
-    return providerLocation.replace('provider.html', 'ui/resolver');
+    return providerLocation.replace('provider.html', 'ui/resolver/index.html');


Maybe a silly question, but do we not have index.* be the default document for the server?

wenjunche and others added 5 commits November 18, 2019 15:33

[OC-1990] launch FDC3 provider and inject client API from fdc3.asar

66c1a5d

[OC-1990]: updated tooling version and added asar command, few lintin…

618826c

…g errors on files I did not change

Merge branch 'develop' into feature/fdc3-asar

ea92b08

review fixes, some spellings

e8b4673

first punt at mr color channels

07f8e48

finos-admin added the cla-present label Dec 19, 2019

Merge branch 'develop' into dev/tim/noodles

3606af7

tomrobertsOF reviewed Jan 6, 2020

View reviewed changes

rypete reviewed Jan 20, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev/tim/noodles #196

Dev/tim/noodles #196

tim-dinsdale commented Dec 19, 2019

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

rypete Jan 20, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

tomrobertsOF Jan 6, 2020

rypete left a comment •

edited

rypete Jan 20, 2020

rypete Jan 20, 2020

rypete Jan 20, 2020

rypete Jan 20, 2020

rypete Jan 20, 2020

Dev/tim/noodles #196

Are you sure you want to change the base?

Dev/tim/noodles #196

Conversation

tim-dinsdale commented Dec 19, 2019

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

rypete left a comment • edited

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

rypete left a comment •

edited