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

chore(javascript): remove dependency to openapitools.json at rollup #323

Merged
merged 3 commits into from
Apr 1, 2022

Conversation

eunjae-lee
Copy link
Contributor

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/DI-211

Changes included:

rollup uses openapitools.json to build js clients, but the file is only in the monorepo. When the rollup runs at algoliasearch-client-javascript repository, it doesn't have access to the file, and the build fails.

This PR copies the file to the js repo on release, but I'm not 100% sure if this is the best way. I'm open to alternative ways. If not, this won't be a terrible idea, though.

@netlify
Copy link

netlify bot commented Apr 1, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 9fa3cb6
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/624708f397f6ec000801cc32

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 1, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the generated/main branch.

@millotp
Copy link
Collaborator

millotp commented Apr 1, 2022

I think having generation related code in client repo is a bad idea, I think it would be better to make rollup.config.js a generated file, because openapitools.json is only used to build the client list line 112 which should be possible to generate.
We might as well hardcode the client list in rollup.config.json rather than copying the whole file from openapi, wdyt ?

@eunjae-lee
Copy link
Contributor Author

I think having generation related code in client repo is a bad idea, I think it would be better to make rollup.config.js a generated file, because openapitools.json is only used to build the client list line 112 which should be possible to generate. We might as well hardcode the client list in rollup.config.json rather than copying the whole file from openapi, wdyt ?

Now that I hear you, what about this? During the release process, we generate a file like release.config.json right next to rollup.config.js. And rollup can import it. The rollup config stays manually written, but only that json file gets updated by the release process. It sounds like the middle ground of two suggestions you just made. WDYT?

@millotp
Copy link
Collaborator

millotp commented Apr 1, 2022

Actually we don't even need an other file, the information is already in the repo, we just need to iterate through the folders name in packages and that the final list (minus client-common, requester-browser-xhr, requester-node-http that can be hardcoded and filtered out)

@eunjae-lee
Copy link
Contributor Author

uh you're right. they're a list of generated clients!

@eunjae-lee eunjae-lee requested a review from millotp April 1, 2022 14:17
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.

Looking great !

@eunjae-lee eunjae-lee changed the title chore(javascript): add openapitools.json to js repository chore(javascript): remove dependency to openapitools.json at rollup Apr 1, 2022
@eunjae-lee eunjae-lee merged commit 0f9a6ba into main Apr 1, 2022
@eunjae-lee eunjae-lee deleted the chore/openapitools branch April 1, 2022 14:42
@eunjae-lee eunjae-lee mentioned this pull request Apr 1, 2022
2 tasks
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Apr 1, 2022
shortcuts pushed a commit that referenced this pull request Apr 22, 2022
…323)

* chore(javascript): add openapitools.json to js repository

* chore: get the list differently
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