Git Product home page Git Product logo

Comments (8)

Flarna avatar Flarna commented on June 15, 2024

Yes, that's true. SDK requires that one of following properties is set in ConnectionInfo, host, socketPath, pipeName or channelType.
I have to think about how I can improve this, not sure if typescript allows such a specification at all. Hints welcome!

from oneagent-sdk-for-nodejs.

dyladan avatar dyladan commented on June 15, 2024

@Flarna Typescript does allow for something like this if you use union types. Something like this:

export type ConnectionInfo = HostPortConnectionInfo | SocketPathConnectionInfo | PipeConnectionInfo | ChannelConnectionInfo;

export interface HostPortConnectionInfo {
  host: string;
  port?: string;
}

export interface SocketPathConnectionInfo {
  socketPath: string;
}

export interface PipeConnectionInfo {
  pipeName: string;
}

export interface ChannelConnectionInfo {
  channelType: ChannelType;
}

This makes it so that one and only one connection parameter is required.

In this particular case I make the assumption that port is optional in the HostPortConnectionInfo, but if that is not the case, simply removing the ? in the port line will make it required.

from oneagent-sdk-for-nodejs.

Flarna avatar Flarna commented on June 15, 2024

I tried that yesterday (with some small changes as you can't create an interface extending from an union type) but it seems to be just have of the truth as it allows to set e.g. host and socketPath but it should be just one of them.
But it's definitely a step forward as it requires at least one.

from oneagent-sdk-for-nodejs.

dyladan avatar dyladan commented on June 15, 2024

A I see the problem. Perhaps a discriminated union?

export enum ConnectionType {
  Host,
  SocketPath,
  Pipe,
  Channel
}

export type ConnectionInfo =
  | HostPortConnectionInfo
  | SocketPathConnectionInfo
  | PipeConnectionInfo
  | ChannelConnectionInfo;

export type HostPortConnectionInfo = {
  type: ConnectionType.Host;
  host: string;
  port: string;
}

export type SocketPathConnectionInfo = {
  type: ConnectionType.SocketPath;
  socketPath: string;
}

export type PipeConnectionInfo = {
  type: ConnectionType.Pipe;
  pipeName: string;
}

export type ChannelConnectionInfo = {
  type: ConnectionType.Channel;
  channelType: ChannelType;
}

const opts: ConnectionInfo = {
  type: ConnectionType.Pipe,
  pipeName: "pipe name here"
}

edit: I guess actually you don't need the enum. A string literal is good enough.

export type ConnectionInfo =
  | HostPortConnectionInfo
  | SocketPathConnectionInfo
  | PipeConnectionInfo
  | ChannelConnectionInfo;

export type HostPortConnectionInfo = {
  type: "host";
  host: string;
  port: string;
}

export type SocketPathConnectionInfo = {
  type: "socketpath";
  socketPath: string;
}

export type PipeConnectionInfo = {
  type: "pipe";
  pipeName: string;
}

export type ChannelConnectionInfo = {
  type: "channel";
  channelType: ChannelType;
}

edit2: looks like interfaces are good enough too in my testing. I don't know which your team typically prefers.

export type ConnectionInfo =
  | HostPortConnectionInfo
  | SocketPathConnectionInfo
  | PipeConnectionInfo
  | ChannelConnectionInfo;

export interface HostPortConnectionInfo {
  type: "host";
  host: string;
  port: string;
}

export interface SocketPathConnectionInfo {
  type: "socketpath";
  socketPath: string;
}

export interface PipeConnectionInfo {
  type: "pipe";
  pipeName: string;
}

export interface ChannelConnectionInfo {
  type: "channel";
  channelType: ChannelType;
}

from oneagent-sdk-for-nodejs.

Flarna avatar Flarna commented on June 15, 2024

I wanted to avoid a discriminated union as user has to set an extra field typewhich is unneeded. if you look into other JS APIs with a similar need (e.g. redis,.. ) something like this is also not done. One target here was to act like others do.
Honestly speaking I have the feeling that typescript should improve the type checking for union types here. I think I will put this together in a small sample and report in the typescript repo.

from oneagent-sdk-for-nodejs.

Flarna avatar Flarna commented on June 15, 2024

maybe related: microsoft/TypeScript#20863

from oneagent-sdk-for-nodejs.

Flarna avatar Flarna commented on June 15, 2024

@dyladan I created PR #2 would be nice if you can take a look.

from oneagent-sdk-for-nodejs.

Flarna avatar Flarna commented on June 15, 2024

Honestly speaking I have the feeling that typescript should improve the type checking for union types here.

Seems this happened with typescript 3.3 👏 👏 👏

from oneagent-sdk-for-nodejs.

Related Issues (14)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.