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.

Sign up for GitHub

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

Jump to bottom

feat(clients): retrieve hosts from spec file #111

Merged
merged 7 commits into from
Feb 2, 2022
Merged

feat(clients): retrieve hosts from spec file #111

merged 7 commits into from
Feb 2, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Jan 31, 2022 •
edited
Loading

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-293

This PR aims at removing manual definition of the additionalProperties in our openapitools.json config file.

We now retrieve options related to the region/hosts from the specs (which is not yet doable via the generator: OpenAPITools/openapi-generator#590 - source code).

Unsuccessful workarounds

  • Using servers in generator: open issue
  • Inline options to CLI: impossible when using an external config file

Changes included:

  • Insights hosts were incorrect, 'de' | 'us' should be accepted.
  • Re-gen sources which was outdated
  • insights, analytics, and abtesting now correctly fallbacks to their host alias.
  • Automatically update the openapitools.json file for the hosts/region related variables before generating the client.

🧪 Test

CI :D

@shortcuts shortcuts requested review from a team and millotp and removed request for a team January 31, 2022 18:38
millotp
millotp reviewed Feb 1, 2022
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Nice !

clients/algoliasearch-client-javascript/client-insights/src/insightsApi.ts Outdated Show resolved Hide resolved
templates/javascript/api-single.mustache Outdated Show resolved Hide resolved
@shortcuts shortcuts force-pushed the fix/APIC-293/javascript-hosts branch from c2313ca to c03732f Compare February 1, 2022 15:24
@shortcuts shortcuts changed the title fix(javascript): fix hosts feat(clients): retrieve hosts from spec file Feb 1, 2022
shortcuts
shortcuts commented Feb 1, 2022
View reviewed changes
openapitools.json
@@ -1,7 +1,7 @@
{
"$schema": "./node_modules/@openapitools/openapi-generator-cli/config.schema.json",
"generator-cli": {
"version": "5.3.0",
"version": "5.4.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I've upgraded when trying inlined options, it was not successful but let's upgrade it anyway

@shortcuts shortcuts marked this pull request as ready for review February 1, 2022 15:35
@shortcuts shortcuts self-assigned this Feb 1, 2022
millotp
millotp reviewed Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Impressive !

scripts/generate.sh Outdated Show resolved Hide resolved
millotp
millotp reviewed Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Beautiful !

base.tsconfig.json Outdated Show resolved Hide resolved
scripts/pre-gen/setHostsOptions.ts Outdated

function setHostsOptions(): void {
const openapitoolsPath = path.join(__dirname, '../../openapitools.json');
const openapitools = JSON.parse(readFileSync(openapitoolsPath, 'utf-8'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can import this one to have the types

Copy link
Member Author

@shortcuts shortcuts Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've done this first but tsc will compile the json file, which changes the path reference etc. In the end, this solution was easier

scripts/pre-gen/setHostsOptions.ts Outdated Show resolved Hide resolved
scripts/pre-gen/setHostsOptions.ts Outdated Show resolved Hide resolved
scripts/pre-gen/setHostsOptions.ts Outdated Show resolved Hide resolved
scripts/pre-gen/setHostsOptions.ts Outdated Show resolved Hide resolved
templates/javascript/api-single.mustache
{{/hasRegionalHost}}
{{#hasRegionalHost}}
public getDefaultHosts(region{{#fallbackToAliasHost}}?{{/fallbackToAliasHost}}: {{#isDeHost}}'de'{{/isDeHost}}{{#isEuHost}}'eu'{{/isEuHost}} | 'us'): Host[] {
{{#fallbackToAliasHost}}const regionHost = region ? `.${region}.` : '.';{{/fallbackToAliasHost}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small detail but I would put one point in the final url and just ${region}. here

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not found a way to make it extra clear, by splitting . I find it confusing so I've kept everything at the same spot

millotp
millotp reviewed Feb 2, 2022
View reviewed changes
scripts/pre-gen/setHostsOptions.ts Outdated
};

writeFileSync(openapitoolsPath, JSON.stringify(openapitools, null, 2));
writeFile(openapitoolsPath, JSON.stringify(openapitools, null, 2));
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing await

Copy link
Member Author

Choose a reason for hiding this comment

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

👀

scripts/pre-gen/setHostsOptions.ts
options.hasRegionalHost = true;
additionalProperties.hasRegionalHost = true;

if (!additionalProperties.fallbackToAliasHost) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

technical ts right here !

millotp
millotp previously approved these changes Feb 2, 2022
View reviewed changes
@shortcuts shortcuts force-pushed the fix/APIC-293/javascript-hosts branch from 928f7e5 to 8e14899 Compare February 2, 2022 15:24
@shortcuts shortcuts force-pushed the fix/APIC-293/javascript-hosts branch from 8e14899 to b91b9c8 Compare February 2, 2022 15:33
@shortcuts
Copy link
Member Author

Opened issue for URL on @types/node: DefinitelyTyped/DefinitelyTyped#19799

Current workarounds: using dom in typescript or importing URL from node. As both are incorrect in our case, I've went with the one will the smallest footprint.

millotp
millotp approved these changes Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

I had no idea URL was not part of classic node !

@shortcuts shortcuts merged commit bf5203e into main Feb 2, 2022
@shortcuts shortcuts deleted the fix/APIC-293/javascript-hosts branch February 2, 2022 17:01
@shortcuts shortcuts mentioned this pull request Feb 11, 2022
Merged
2 tasks
shortcuts added a commit that referenced this pull request Apr 22, 2022
@shortcuts
feat(clients): retrieve hosts from spec file (#111)
764af21
This was referenced May 24, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millotp millotp millotp approved these changes

Assignees

@shortcuts shortcuts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shortcuts @millotp