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(script): use typescript for all scripts APIC-334 #170

Merged
merged 21 commits into from
Feb 25, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Feb 23, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-334

Replace bash file by a beautiful typescript cli

Everything is called from one command, yarn api, or yarn docker to run it under docker (mandatory for local),
with a helper and completion if you don't provide the arguments on local (maybe overkill). On CI it defaults to all.

ts-node is a bit too slow, it takes 9 seconds to launch, 6 with --transpile-only and 12 with swc (I think it's because of docker, it should be way faster). Maybe we could compile it and just standard js, but we don't want to build before each command, and ts-node gives us more visibility when failing, with error in ts source code.

Improvements

  • verbose could be global instead of passed to each function
  • more tests
  • more colors
  • clearer logging

🧪 Test

yarn docker all commands

@millotp millotp self-assigned this Feb 23, 2022
@millotp millotp changed the title chore(tooling): use typescript for all scripts APIC-334 feat(script): use typescript for all scripts APIC-334 Feb 24, 2022
@eunjae-lee
Copy link
Contributor

include me as reviewer when ready for review.

@millotp millotp marked this pull request as ready for review February 24, 2022 17:18
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

First small review, will do an other batch soon

Good initiative :D I prefer that 100000000% GG

README.md Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We definitely need a config folder

package.json Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildClients.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/index.ts Show resolved Hide resolved
@eunjae-lee
Copy link
Contributor

Can we have another script name than yarn api? It feels like it's the script regarding the "api" clients.

  • yarn script
  • yarn cli
  • ...?

@eunjae-lee
Copy link
Contributor

did you know i was going to comment that, or did you make that change in the speed of light? 😂

@millotp
Copy link
Collaborator Author

millotp commented Feb 25, 2022

ahah @shortcuts already proposed something similar #170 (comment)

@eunjae-lee
Copy link
Contributor

ahah @shortcuts already proposed something similar #170 (comment)

ahah I missed it lol

{ language, key, additionalProperties: { packageName } }: Generator,
verbose: boolean
): Promise<void> {
const spinner = createSpinner(`building ${key}`, verbose).start();
Copy link
Contributor

@eunjae-lee eunjae-lee Feb 25, 2022

Choose a reason for hiding this comment

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

it seems we don't want to call this function for non-js languages. what if we add throw just in case it's used wrongly in the future?

It's not exported, so it won't matter much. It's still easier to read if there's a short comment here and for buildAllClients.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

that is stylé

(need a break from review, brb in a few mins D:)

README.md Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/buildSpecs.ts Outdated Show resolved Hide resolved
scripts/common.ts Outdated Show resolved Hide resolved
scripts/cts/client/generate.ts Show resolved Hide resolved
scripts/cts/client/generate.ts Show resolved Hide resolved
scripts/tsconfig.json Show resolved Hide resolved
scripts/release/process-release.ts Show resolved Hide resolved
scripts/release/create-release-issue.ts Show resolved Hide resolved
scripts/release/create-release-issue.ts Show resolved Hide resolved
// generate clients to release
for (const lang of Object.keys(versionsToRelease)) {
console.log(`Generating ${lang} client(s)...`);
await run(`yarn cli generate ${lang}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to give verbose: true here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

verbose shows the output of openapi generator and formatter which is very verbose, it should only be used when debugging IMO

scripts/release/process-release.ts Show resolved Hide resolved
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Really nice :D Time to do some further testing!!

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

Successfully merging this pull request may close these issues.

3 participants