docs: moving forward with this library

KhudaDad414 commented 3 years ago

Reason/Context

Fortunately, the project is initialized and we can discuss the API of this library here.

Description

Here is a draft of the API as well as the proposed Report structure. After finalizing the API and report structure, I will open a pull request with these changes in the README.md file.

Usage

Node.js

import { Optimizer } from '@asyncapi/optimizer';
import { parse } from '@asyncapi/parser';
let asyncApiDocument = parse(`
asyncapi: 2.0.0
info:
  title: Streetlights API
  version: '1.0.0'

channels:

  smartylighting/event/{streetlightId}/lighting/measured:
    parameters:
      #this parameter is duplicated. it can be moved to components and ref-ed from here.
      streetlightId:
        schema:
          type: string
    subscribe:
      operationId: receiveLightMeasurement
      traits:
        - bindings:
            kafka:
              clientId: my-app-id
      message:
        name: lightMeasured
        title: Light measured
        contentType: application/json
        traits:
          - headers:
              type: object
              properties:
                my-app-header:
                  type: integer
                  minimum: 0
                  maximum: 100
        payload:
          type: object
          properties:
            lumens:
              type: integer
              minimum: 0
            #full form is used, we can ref it to: #/components/schemas/sentAt
            sentAt:
              type: string
              format: date-time

  smartylighting/action/{streetlightId}/turn/on:
    parameters:
      streetlightId:
        schema:
          type: string
    publish:
      operationId: turnOn
      traits:
        - bindings:
            kafka:
              clientId: my-app-id
      message:
        name: turnOnOff
        title: Turn on/off
        traits:
          - headers:
              type: object
              properties:
                my-app-header:
                  type: integer
                  minimum: 0
                  maximum: 100
        payload:
          type: object
          properties:
            sentAt:
              $ref: "#/components/schemas/sentAt"

components:
  messages:
    #libarary should be able to find and delete this message because it is not used anywhere.
    unusedMessage:
      name: unusedMessage
      title: This message is not used in any channel.

  schemas:
    #this schema is ref-ed in one channel and used full form in another. the library should be able to identify and ref the second channel as well.
    sentAt:
      type: string
      format: date-time`
);
let optimizer = new Optimizer(asyncApiDocument);
let report: OptimizerReport = await optimizer.getReport();
/*
the `report` value will be:
{
  ReuseComponents: [
    {
      path: '#/channels/channel1/smartylighting/event/{streetlightId}/lighting/measured/message/payload/properties/sentAt',
      action: 'ref',
      refTo: '#/components/schemas/sentAt'
    }
  ],
  RemoveComponents: [
    {
      path: '#/components/messages/unusedMessage',
      action: 'remove',
    }
  ],
  MoveToComponents: [
    {
      //move will ref the current path to the moved component as well.
      path: '#/channels/smartylighting/event/{streetlightId}/lighting/measured/parameters/streetlightId',
      action: 'move',
      moveTo: '#/components/parameters/streetlightId'
    },
    {
      path: '#/channels/smartylighting/action/{streetlightId}/turn/on/parameters/streetlightId',
      action: 'ref',
      refTo: '#/components/parameters/streetlightId'
    }
  ]
}
 */
let optimizedDocument = optimizer.getOptimizedDocument({rules: {ReuseComponents: true,RemoveComponents: true,MoveToComponents: true }})
/*
the `optimizedDocument` value will be:

asyncapi: 2.0.0
info:
  title: Streetlights API
  version: '1.0.0'

channels:

  smartylighting/event/{streetlightId}/lighting/measured:
    parameters:
      streetlightId:
        $ref: "#/components/schemas/streetlightId"
    subscribe:
      operationId: receiveLightMeasurement
      traits:
        - bindings:
            kafka:
              clientId: my-app-id
      message:
        name: lightMeasured
        title: Light measured
        contentType: application/json
        traits:
          - headers:
              type: object
              properties:
                my-app-header:
                  type: integer
                  minimum: 0
                  maximum: 100
        payload:
          type: object
          properties:
            lumens:
              type: integer
              minimum: 0
            #full form is used, we can ref it to: #/components/schemas/sentAt
            sentAt:
              $ref: "#/components/schemas/sentAt"

  smartylighting/action/{streetlightId}/turn/on:
    parameters:
      streetlightId:
        $ref: "#/components/schemas/streetlightId"
    publish:
      operationId: turnOn
      traits:
        - bindings:
            kafka:
              clientId: my-app-id
      message:
        name: turnOnOff
        title: Turn on/off
        traits:
          - headers:
              type: object
              properties:
                my-app-header:
                  type: integer
                  minimum: 0
                  maximum: 100
        payload:
          type: object
          properties:
            sentAt:
              $ref: "#/components/schemas/sentAt"

components:
  parameters:
    streetlightId:
      schema:
      type: string
  schemas:
    #this schema is ref-ed in one channel and used full form in another. library should be able to identify and ref the second channel as well.
    sentAt:
      type: string
      format: date-time`
 */

API

Constructor

new Optimizer(document)

document is a mandatory object that is parsed with @asyncapi/parser:

Methods

getReport() : OptimizerReport

getOptimizedDocument([options]) : string

options: this is an object that contains all of the options that we can pass in getOptimizedDocument. right now we have only rules here but maybe in the future we can have other options as well.
- rules: using rules object different optimization types can be enabled or disabled.
- ReuseComponents if set to true, optimizer will apply all of ReuseComponents changes from report. (default: true)
- RemoveComponents if set to true, optimizer will apply all of RemoveComponents changes from report. (default: false)
- MoveToComponents if set to true, optimizer will apply all of MoveToComponents changes from report. (default: true)

github-actions[bot] commented 3 years ago

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

KhudaDad414 commented 3 years ago

@derberg @jazzyarchitects @magicmatatjahu @ Any input would be appreciated especially with report object structure.

magicmatatjahu commented 3 years ago

@KhudaDad414 Great work! Some my thoughts:

please don't use capitalized names for options ReuseComponents, RemoveComponents etc. it should be started with lower reuseComponents. In JS only classes, const, global variables should start with with upper case. It should be done for options and also for fields in return value of getReport function - in report object
I very like approach with shape of items in the result object like:

      path: '#/channels/channel1/smartylighting/event/{streetlightId}/lighting/measured/message/payload/properties/sentAt',
      action: 'ref',
      refTo: '#/components/schemas/sentAt'

but I have two problems:

should we use reuse than ref to be consistent with other action names? Ref is better for me, but I only ask for consistency. Lets see what other people think.
I don't think so that we should merge in the "move" array the items with ref. I think that we should add these refs in the reuse array but with some description/annotation something like: Ref is created from move array etc. :)

I hope that options for rules will be merged with default ones and passed by user. Here you can use https://github.com/jonschlinkert/merge-deep package to deep merge.
I'm also thinking if we should make all methods as async functions. If someone will have several specs to optimize, then be able to optimize every document in parallel.
new Optimizer(document) document is a mandatory object that is parsed with @asyncapi/parser:

I think that we should by default also parse spec if someone pass normal yaml or json. There is an example of function how to check if input doc is parsed schema or not. If user will pass not parsed schema but pure spec, then you will have to parse it :)

derberg commented 3 years ago

awesome stuff:

agree with @magicmatatjahu on options names
definitely love the structure of the report object!
same concerns on reuse and ref. When I looked at the report first, I was like, "this needs to be consistent" and then I saw the comment from @magicmatatjahu. I always value consistency more so prefer reuse
I'm actually ok that ref was in the same location as move because it is clear what change was done for a given option. It will be confusing for the users when they will decide to use only moveToComponents and ref will not be in the report.
If someone will have several specs to optimize, then be able to optimize every document in parallel I don't think we need to care atm because the main use case is that users do it in a pipeline for a single document. For batch optimizations, we need a real use case imho.
I think that we should by default also parse spec this is optional, for or the GSoC related libraries, when not needed, I said skip parser because the current intention is that these are all libraries for the UI and CLI where AsyncAPI docs are already parsed

magicmatatjahu commented 3 years ago

I agree with Łukasz, these two last aren't need, we can make issues and see if someone from community will want this features :)

jazzyarchitects commented 3 years ago

Nice report structure @KhudaDad414. In addition to the consistency part mentioned in the previous comments, IMO it would be more awesome if the objects in each of the three arrays have the same structure. One of the objects have moveTo field and one has refTo. While this might not be a deal breaking thing, from what I have seen, having a single structure format helps when the output is consumed or parsed by some strictly typed languages.

Secondly, would it be a better parsing experience from a consumers point to have only a single array and segregate based on different actions (@derberg , @magicmatatjahu would like to hear your thoughts on this)?

jazzyarchitects commented 3 years ago

Another suggestion, how about a chained interface?

const changes =  await optimizer.getReport(); // Which can return an object of say `Report` class
changes.apply({rules: })

KhudaDad414 commented 3 years ago

it would be more awesome if the objects in each of the three arrays have the same structure. One of the objects have moveTo field and one has refTo

as you suggested, for consistency it would be a good idea to change moveTo and refTo to another name like target. In that case, if we have another type of optimization in the future, it can use path, action, target structure.

magicmatatjahu commented 3 years ago

Secondly, would it be a better parsing experience from a consumers point to have only a single array and segregate based on different actions.

@jazzyarchitects It's a some idea, but there is a one problem: if someone will operate on this array, will have to check on every item the kind of operation - move to, ref to etc. even if array will be segregated, so object with reuse..., remove... move... etc is a better shape for me, but of course I don't think so that it will be hard to change it in the future, so we can discuss about it :)

Another suggestion, how about a chained interface?

There is a some solution. I have no opinion on this, because both the chained functions and normal functions will be good here.

as you suggested, for consistency it would be a good idea to change moveTo and refTo to another name like target. In that case, if we have another type of optimization in the future, it can use path, action, target structure.

@KhudaDad414 Please, don't forget about document (of course in the next PRs) the shape of object in the docs/Readme.md, especially target :)

derberg commented 3 years ago

How about:

{
  ReuseComponents: [
    {
      path: '#/channels/channel1/smartylighting/event/{streetlightId}/lighting/measured/message/payload/properties/sentAt',
      refTo: '#/components/schemas/sentAt'
    }
  ],
  RemoveComponents: [
    {
      path: '#/components/messages/unusedMessage',
      remove: true
    }
  ],
  MoveToComponents: [
    {
      path: '#/channels/smartylighting/event/{streetlightId}/lighting/measured/parameters/streetlightId',
      moveTo: '#/components/parameters/streetlightId'
    },
    {
      path: '#/channels/smartylighting/action/{streetlightId}/turn/on/parameters/streetlightId',
      refTo: '#/components/parameters/streetlightId'
    }
  ]
}

I just had a thought: what's the point of having action if I can figure it out through the key name

anyway, I think the best way is to think about it from the point of view of the user that will read the report in the UI and the JS code that will parse the report. In the end, you will not print the Map to the user, right?

as a user, I want to:

have all optimizations grouped by the rule name so I know which rule I can ignore
I want to see how my document will look like after applying all the rules (will use getOptimizedDocument) + I want a view of change one by one, so not only JSON pointers but visually see the specific part of the document before and after, to understand the change

now, is it better if it will be no action or there will be action, or there will be ref or reuse? you will figure it out the best during integrating the library in CLI or UI.

Conclusion: I suggest pick the best structure you think based on our feedback, implement, and then integrate and make improvements that will make your integration code nicer 😄 The good thing is that it is a library in the development phase, the structure of the report may change frequently, so better I think is to pick something that you think should be good and then iterate improvements.

Thoughts?

asyncapi / optimizer