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

Include Operations in the index.ts file #856

Closed
sarangan12 opened this issue Feb 1, 2021 · 9 comments
Closed

Include Operations in the index.ts file #856

sarangan12 opened this issue Feb 1, 2021 · 9 comments
Assignees

Comments

@sarangan12
Copy link
Member

Refer the index.ts file here.

export * from "./models";
export { AdditionalPropertiesClient } from "./additionalPropertiesClient";
export { AdditionalPropertiesClientContext } from "./additionalPropertiesClientContext";

There is no export from operations folder.

But, if you look at the code in the additionalPropertiesClient - here there are references to classes from the Operations folder.

If the operations folder is not exported, then the SDK users will not have any method to access them directly. Also, you can see a forgotten-export error in the API Extractor.

So, we need to fix this error by adding operations export in the index.ts file.

Note: This operations export is not required for packages (such as noLicenseHeader), where there is no operations folder.

@joheredi @deyaaeldeen @ramya-rao-a FYI......

@qiaozha
Copy link
Member

qiaozha commented Feb 4, 2021

I had similar problem in mgmt plane SDK.
I figured out a way to include the operation in track1 mgmt SDK, which is to change the src/index.ts like this

 /// <reference lib="esnext.asynciterable" />
export * from "./models";
export * from "./operations";
export { ComputeManagementClient } from "./computeManagementClient";
export { ComputeManagementClientContext } from "./computeManagementClientContext";

In the api extractor result, you will see the these operations are included.
image

But when I tried to add this line export * from "./operations"; in track2 mgmt SDK src/index.ts, it reports error about duplicate export.
For example, when I use compute RP,
image

@qiaozha
Copy link
Member

qiaozha commented Feb 4, 2021

cc: @ramya-rao-a

@joheredi
Copy link
Member

joheredi commented Feb 8, 2021

@qiaozha, I would like to understand the scenario where the individual Operation classes are needed by themselves. Would you mind sharing some context on when the user would need to instantiate the Operation class?

We purposefully decided not to export these classes as we couldn't find a scenario where they would be needed, keeping in mind that the Client itself is needed to instantiate them, and the client includes these operations as members.

However, if we find valid scenarios to have them exported we'll be happy to, just want to make sure we understand why.

If the reason for exporting these is to have APIExtractor not have a warning, I think we should create interfaces for the Operations, have the operation classes implement those interfaces and switch to interfaces instead of the concrete classes when refferencing them

@qiaozha
Copy link
Member

qiaozha commented Feb 9, 2021

@joheredi Actually I intend to use this api extractor result for changelog tools, the main focus on changelog would be api level changes.

@qiaozha
Copy link
Member

qiaozha commented Feb 9, 2021

if purely exposing interface like this

// @public
export interface SshPublicKey {
    keyData?: string;
    path?: string;
}

We don't know which apis are impacted by this change. Also customers may care about the api signature etc.

@qiaozha
Copy link
Member

qiaozha commented Feb 22, 2021

cc @lirenhe

@joheredi
Copy link
Member

Currently the Client class is defined as follows:

import {  Basic } from "./operations";
export class BodyComplexClient extends BodyComplexClientContext {
  /**
   * Initializes a new instance of the BodyComplexClient class.
   * @param options The parameter options
   */
  constructor(options?: BodyComplexClientOptionalParams) {
    super(options);
    this.basic = new Basic(this);
  }

  basic: Basic;
}

Where the members of the BodyComplexClient are the concrete Operation classes.

The main indext.ts file doesn't export the Basic from operations, which causes api-extractor to warn about a missing export.

My proposal is to change a bit the definition of the Client class, in which their members won't be of the concrete class type but an interface instead. Fore example:

  1. Define the interface for the operations
// operations/interfaces.ts
export interface Basic {
   getValid(options?: coreHttp.OperationOptions): Promise<BasicGetValidResponse>
}
  1. Change the operation class to implement the interface
// operations/basic.ts
import { Basic } from "./interfaces";
export class BasicImpl implements Basic {
   constructor(client: BodyComplexClientContext) {
      this.client = client;
   }
}
  1. Make the Client class have its members be the operation interface
// bodyComplexClient.ts
import {  Basic, BasicImpl } from "./operations";
export class BodyComplexClient extends BodyComplexClientContext {
  /**
   * Initializes a new instance of the BodyComplexClient class.
   * @param options The parameter options
   */
  constructor(options?: BodyComplexClientOptionalParams) {
    super(options);
    this.basic = new BasicImpl(this);
  }

  basic: Basic;
}
  1. Finally export the operation interfaces
// index.ts
export * from "./models";
export { BodyComplexClient } from "./bodyComplexClient";
export { BodyComplexClientContext } from "./bodyComplexClientContext";
export * from "./operations/interfaces"; 

With this change we'll now be exporting everything that needs to be exported and api-extractor will be happy.

We don't want to export the operation classes as they provide no value by themselves. SDK consumers can get any of the operations through the client.

@qiaozha
Copy link
Member

qiaozha commented Mar 15, 2021

I am fine with this proposal. Just wondering if it's going to be a big change in the code structure?

@sarangan12
Copy link
Member Author

Code complete with PR #890

@sarangan12 sarangan12 self-assigned this Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants