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

feat: add Modelina configuration file support #430

Closed

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jan 16, 2023

Description
Currently, it's hard to utilize the full feature suite of Modelina, because they are hidden behind callbacks (to allow full customization of model rendering). To enable these features we need to support a basic configuration file, that the user can use to customize the model generation phase when running the asyncapi generate models command.

This PR is just a proof of concept to be used for discussing how to proceed.

To enable the configuration file a user can do the following:

asyncapi generate models typescript -c="./typescript_model.js"

Where the configuration file is:

const {TS_COMMON_PRESET, TypeScriptOptions, IndentationTypes} = require('@asyncapi/modelina');
/** @type {TypeScriptOptions} */
module.exports = {
  enumType: 'union',
  modelType: 'interface',
  indentation: {
    size: 10,
    type: IndentationTypes.SPACES
  },
  mapType: 'record',
  renderTypes: false,
  moduleSystem: 'CJS',
  constraints: {
    modelName: (context) => {
      return `Custom${context.modelName}`;
    },
    propertyKey: (context) => {
      return `custom_prop_${context.objectPropertyModel.propertyName}`;
    }
  },
  typeMapping: {
    Any: (context) => {
      // Always map AnyModel to number
      return 'number';
    }
  },
  presets: [
    {
      preset: TS_COMMON_PRESET,
      options: {
        example: true
      }
    }
  ]
}

This gives 100% control of the generators to the user.

Related issue(s)
Related to asyncapi/modelina#780

@jonaslagoni jonaslagoni marked this pull request as draft January 16, 2023 10:35
@sonarcloud
Copy link

sonarcloud bot commented Jan 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jan 16, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

@fmvilas
Copy link
Member

fmvilas commented Jan 16, 2023

I love the approach. Gives a ton of flexibility. Have you thought about how to handle dependencies? I mean, what if in this file I want to import one dependency like lodash? Where should it go? A local package.json might be too much given that I may not even be running the command from a Node.js project or a project at all 🤔 Maybe scanning the file first to install the dependencies in their latest version? 🤔

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 16, 2023

I actually did not think about that, but definitely a valid concern 🤔 Hmm...

At the point where you need to create a node project to run the command, do you not think it will be easier to add your own integration instead of using the CLI? So you have a script such as:

import { TypeScriptGenerator, TS_COMMON_PRESET } from '@asyncapi/modelina';
import SomeDependency from 'something';

const generator = new TypeScriptGenerator({
  presets: [
    {
      preset: TS_COMMON_PRESET,
      options: {
        example: true
      }
    }
  ]
});

export async function generate(): Promise<void> {
  const models = await generator.generate(input);
  for (const model of models) {
    console.log(model.result);
  }
}
generate();

When we extend this feature to the AsyncAPI generator and templates, adding a solution that enables dependencies would be needed anyway... Might need to take a page out of the AsyncAPI generator 🤔

Might need to add some helpers in Modelina to lift the load with this then 🤔 But I guess we can let it sit in the CLI for now until we have a working solution.

@derberg
Copy link
Member

derberg commented Jan 16, 2023

You could always provide a clear instruction on how one can bundle dependencies into the config script. This is how GitHub Actions work now. You bundle all the code into one file that GitHub workflow uses and dist code. So you have instruction, if you want config file to contain some external dependencies then install them and then bundle with @vercel/ncc.

Anyway it is more on modelina side, right? not cli. You provide guide there, and just add it as link in description: 'Path to config file'

@jonaslagoni
Copy link
Member Author

You could always provide a clear instruction on how one can bundle dependencies into the config script. This is how GitHub Actions work now. You bundle all the code into one file that GitHub workflow uses and dist code. So you have instruction, if you want config file to contain some external dependencies then install them and then bundle with @vercel/ncc.

Can you clarify a bit more about what you mean? How bundling dependencies and code could work?

Anyway it is more on modelina side, right? not cli. You provide guide there, and just add it as link in description: 'Path to config file'

Yea I think so too, see asyncapi/modelina#780

But I want to see what functionality overlaps between CLI and Generator integrations before placing the logic in Modelina. Because arguments and the configuration file are overlapping in the CLI and the generator will probably have multiple configuration files (TBD), so I would rather not place common logic in Modelina before actually having proof it's needed.

I think the best way is to slowly progress and see what works and what does not. Always easy to remove stuff once we see commonalities 🙂

@derberg
Copy link
Member

derberg commented Jan 16, 2023

Can you clarify a bit more about what you mean? How bundling dependencies and code could work?

  1. you create this:
import { TypeScriptGenerator, TS_COMMON_PRESET } from '@asyncapi/modelina';
import SomeDependency from 'something';

const generator = new TypeScriptGenerator({
  presets: [
    {
      preset: TS_COMMON_PRESET,
      options: {
        example: true
      }
    }
  ]
});

export async function generate(): Promise<void> {
  const models = await generator.generate(input);
  for (const model of models) {
    console.log(model.result);
  }
}
generate();
  1. then you install dependencies
  2. then you install vercel bundler and run ncc build your-config.js --external="@asyncapi/modelina" (my assumption is that you do not want to bundle modelina in the bundle as it runs inside modelina anyway 😄 )
  3. use generated index.js as the config that you pass to CLI

of course I did not test ☝🏼 with modelina and cli. Purely theoretical idea 😄


Because arguments and the configuration file are overlapping in the CLI and the generator will probably have multiple configuration files (TBD)

sorry I didn't get this. What overlaps, the -c that you add on CLI? with what exactly?

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 16, 2023

of course I did not test ☝🏼 with modelina and cli. Purely theoretical idea 😄

Ahh okay got it! Yea that is definitely another solution 👍

sorry I didn't get this. What overlaps, the -c that you add on CLI? with what exactly?

Oftentimes configuration files are used as the baseline, and then arguments overwrite it i.e. a user could do:

asyncapi generate models typescript -c="./typescript_model.js" --tsModelType="interface"

So even if the config file specified outputting the models as class, the argument for the CLI takes priority.

Copy link
Member

derberg commented Jan 17, 2023

Oftentimes configuration files are used as the baseline, and then arguments overwrite it i.e. a user could do:

ah yes, but this can be solved by making these mutually exclusive. So if you use config, other props are ignored.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label May 18, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonaslagoni There has been a recent change in the folder structure for tests in #712. This can be moved into https://github.com/asyncapi/cli/tree/master/test/fixtures.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, this and other files can be moved into the new folder. https://github.com/asyncapi/cli/tree/master/test/integration/generate

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the tests have been shifted to mocha now so some functions need to be changed.

@github-actions github-actions bot removed the stale label Sep 13, 2023
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jan 31, 2024
@Amzani
Copy link
Collaborator

Amzani commented May 14, 2024

@Shurtu-gal @jonaslagoni is this PR still needed?

@github-actions github-actions bot removed the stale label May 15, 2024
@Shurtu-gal
Copy link
Collaborator

@Shurtu-gal @jonaslagoni is this PR still needed?

I don't think so as modelina has its own CLI too. WDYT @jonaslagoni.

@jonaslagoni
Copy link
Member Author

Its not needed no, once #1376 is merged it will automatically be included once we add it in Modelina 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants