refactor: Migrate the openapi-generator from oclif to yargs #1661

florian-richter · 2021-10-08T14:01:26Z

This refactoring removes oclif and cli-ux in favor of yargs. There should be no changes in functionality or usage. There are some minor changes in behavior during edge cases and tiny changes in the generated help description.

I also changed the github actions to run with node14 (current LTS) rather than node12. To make it possible to merge this PR, I removed the requirement for the node12 tests to pass. After this is merged (or if you disagree with this change), the requirement should be added again (with the matching node version).

TODO:

Fix tests
Generate README
Test end to end

Closes SAP/cloud-sdk-backlog#280

# Conflicts: # yarn.lock

florian-richter · 2021-10-14T12:01:36Z

.github/workflows/build.yml

@@ -15,7 +15,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest]
-        node-version: [12.x]


Node 12 is in maintenance mode until Apr2022. Therefore I assume we should be okay to test with node 14 instead. This change is required to use fs/promises which are used in the generate-readme.ts script below. If this is a problem, we could replace fs/promises with utils.promisify or fs-extra instead. Just let me know.

I think that was planned anyways for 2.0, right? Please document this in the new changelog-v2.md

Anyways, maybe next time this could be a separate PR (for easier reverting if something goes wrong).

We planned to migrate to ES19 as it is supported by node14 and above. Since the readme script is not customer facing, this still means that we are node12 compatible but are no longer testing it explicitly...
You got a point with this being a separate PR. Should I do this already? I don't expect too much effort for splitting.

marikaner

Looks very good!

marikaner · 2021-10-14T16:50:59Z

.github/workflows/build.yml

@@ -15,7 +15,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest]
-        node-version: [12.x]


I think that was planned anyways for 2.0, right? Please document this in the new changelog-v2.md

marikaner · 2021-10-14T16:52:03Z

.github/workflows/build.yml

@@ -15,7 +15,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest]
-        node-version: [12.x]


Anyways, maybe next time this could be a separate PR (for easier reverting if something goes wrong).

marikaner · 2021-10-14T16:57:55Z

packages/openapi-generator/src/cli/index.ts

    }
+  } catch (err) {
+    logger.error(err);
+    yargs.exit(1, new Error());


No error message? Is this intended?

We log the error with the logger in line 30. yargs itself does not handle our "errorWithCause" as we intend.

Ok, maybe just add a short comment. If we exit with 1 and log an error before, it seems unnecessary to pass an empty error there.

I agree but this was the only way I found to make it work due to the typing of yargs.exit(). I'm open to suggestions, but maybe we should just add it to the follow up ticket instead.

marikaner · 2021-10-14T16:58:25Z

packages/openapi-generator/src/generator.spec.ts

@@ -82,26 +82,26 @@ describe('generator', () => {
      mock.restore();
    });

-    it('should transpile the generated sources', async () => {
-      const files = await promises.readdir(outputPath);
+    // it('should transpile the generated sources', async () => {


Comment it back in? ;)

Whoops 😄 good catch

packages/openapi-generator/src/generator.spec.ts

packages/openapi-generator/src/options/flags.ts

CHANGELOG.md

marikaner · 2021-10-14T17:09:26Z

packages/openapi-generator/src/options/flags.ts

+    coerce: (input?: string): string | undefined => {
+      if (typeof input !== 'undefined') {
+        const isFilePath =
+          (existsSync(input) && lstatSync(input).isFile()) || !!extname(input);
+        return isFilePath
+          ? resolve(input)
+          : resolve(input, 'options-per-service.json');
+      }


Does this also work with async functions? I know that it didn't work with oclif, that is why it is implemented with sync functions.

To my understanding it does, but it causes the parsed type to be a promise i think... I would rather keep this change out of this initial ticket.

Makes sense

marikaner · 2021-10-14T17:10:33Z

packages/openapi-generator/src/options/generator-options.ts

  config?: string;
 }

 /**
 * Parsed options with default values.
 */
-export type ParsedGeneratorOptions =
-  typeof OpenApiGenerator extends Parser.Input<infer F> ? F : never;
+export interface ParsedGeneratorOptions {


I assume with yargs there is no type that we can derive?

As discussed, can be part of a follow up, but this is low prio

packages/openapi-generator/src/options/generator-options.ts

marikaner

LGTM

florian-richter · 2021-10-15T14:26:25Z

Follow up ticket: https://github.com/SAP/cloud-sdk-backlog/issues/402

* Add yargs options * Fix types * Fix typo bug in parseGeneratorOptions * Fix config handling * Remove oclif and cli-ux * Improve types * Replace parsedGeneratorOptions * Fix tests * Fix tests * Revert change to cli option 'include' * Apply coerce if not using argv * Differentiate between GeneratorOpts and ParsedOpts * Fix tests (hopefully) * Revert change to options in generate-test-services * Debug integration tests * Add shebang * Fix tests * Fix typos * Fix integration tests * Fix tests * Use stricter version for @SAP packages * Generate readme * Add note to changelog * Use node 14 instead of 12 in GHActions * Revert node14 to node12 * Move index.ts to cli.ts * Rename flags.ts to options.ts # Conflicts: # yarn.lock

Florian Richter added 28 commits September 28, 2021 13:21

Add yargs options

84248c0

Fix types

6827a6d

Fix typo bug in parseGeneratorOptions

d25a1c1

Fix config handling

306dd85

Merge branch 'main' into yargs-openapi-generator

8429f51

# Conflicts: # yarn.lock

Remove oclif and cli-ux

8422d4e

Improve types

0e9e85e

Replace parsedGeneratorOptions

d3079dc

Merge branch 'main' into yargs-openapi-generator

a8e37a7

# Conflicts: # yarn.lock

Fix tests

da3e328

Fix tests

e071b06

Revert change to cli option 'include'

63be9dd

Merge branch 'main' into yargs-openapi-generator

a87c626

Apply coerce if not using argv

9189da7

Differentiate between GeneratorOpts and ParsedOpts

36f8b3d

Fix tests (hopefully)

be5c793

Revert change to options in generate-test-services

84e05a5

Debug integration tests

0bc286d

Merge branch 'main' into yargs-openapi-generator

3cd379d

# Conflicts: # yarn.lock

Merge branch 'main' into yargs-openapi-generator

9006e4d

Add shebang

c32dc85

Fix tests

7aba91e

Fix typos

5b00538

Merge branch 'main' into yargs-openapi-generator

94edc9a

# Conflicts: # yarn.lock

Fix integration tests

eda744d

Fix tests

2345779

Merge branch 'main' into yargs-openapi-generator

98afcdd

Use stricter version for @SAP packages

36f87c5

florian-richter marked this pull request as ready for review October 13, 2021 11:51

florian-richter requested a review from marikaner October 13, 2021 12:19

florian-richter requested review from jjtang1985 and FrankEssenberger October 13, 2021 12:19

Florian Richter added 4 commits October 14, 2021 11:02

Generate readme

5f2f238

Merge branch 'main' into yargs-openapi-generator

de69310

# Conflicts: # yarn.lock

Add note to changelog

400cb64

Use node 14 instead of 12 in GHActions

7b80638

florian-richter commented Oct 14, 2021

View reviewed changes

marikaner reviewed Oct 14, 2021

View reviewed changes

Florian Richter added 2 commits October 15, 2021 11:25

Revert node14 to node12

b9c9568

Move index.ts to cli.ts

5461604

marikaner approved these changes Oct 15, 2021

View reviewed changes

Rename flags.ts to options.ts

b8df407

florian-richter merged commit ccbca93 into main Oct 15, 2021

florian-richter deleted the yargs-openapi-generator branch October 15, 2021 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Migrate the openapi-generator from oclif to yargs #1661

refactor: Migrate the openapi-generator from oclif to yargs #1661

florian-richter commented Oct 8, 2021 •

edited

florian-richter Oct 14, 2021

marikaner Oct 14, 2021

marikaner Oct 14, 2021

florian-richter Oct 15, 2021

marikaner left a comment

marikaner Oct 14, 2021

marikaner Oct 14, 2021

marikaner Oct 14, 2021

florian-richter Oct 15, 2021

marikaner Oct 15, 2021

florian-richter Oct 15, 2021

marikaner Oct 14, 2021

florian-richter Oct 15, 2021

marikaner Oct 14, 2021

florian-richter Oct 15, 2021 •

edited

marikaner Oct 15, 2021

marikaner Oct 14, 2021

marikaner Oct 15, 2021

marikaner left a comment

florian-richter commented Oct 15, 2021

refactor: Migrate the openapi-generator from oclif to yargs #1661

refactor: Migrate the openapi-generator from oclif to yargs #1661

Conversation

florian-richter commented Oct 8, 2021 • 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

marikaner left a 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

florian-richter Oct 15, 2021 • 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

marikaner left a comment

Choose a reason for hiding this comment

florian-richter commented Oct 15, 2021

florian-richter commented Oct 8, 2021 •

edited

florian-richter Oct 15, 2021 •

edited