Skip to content

docs: note on exposed dependencies #428

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

Merged
merged 6 commits into from
Apr 26, 2022
Merged

docs: note on exposed dependencies #428

merged 6 commits into from
Apr 26, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Apr 25, 2022

🧭 What and Why

State that we should not expose dependencies in our client (this is not yet the case for Java)

@millotp millotp requested a review from a team April 25, 2022 16:56
@millotp millotp self-assigned this Apr 25, 2022
@millotp millotp requested review from eunjae-lee and shortcuts and removed request for a team April 25, 2022 16:56
@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 25, 2022

✗ The generated branch has been deleted.

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

@netlify
Copy link

netlify bot commented Apr 25, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 33bad8a
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6266d2e42d610e0008f6786a
😎 Deploy Preview https://deploy-preview-428--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@millotp millotp changed the title doc: note on exposed dependencies docs: note on exposed dependencies Apr 25, 2022
@shortcuts
Copy link
Member

Glad to see the netlify bot commenting twice like ours did :D No need to fix it

@algolia algolia deleted a comment from netlify bot Apr 26, 2022
@@ -88,6 +88,15 @@ Then the resulting User Agent is `Algolia for Java (5.0.0); Search (5.0.0); JVM

You can take a look at the Java implementation [here](https://github.com/algolia/api-clients-automation/pull/347).

### Dependencies

You can use any dependency you want to create the client, it can be Json parser or HTTP client, but it's important to never expose those dependencies through the client, meaning:
Copy link
Member

Choose a reason for hiding this comment

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

The reason behind this could be added

- a function cannot accept an object from a dependency as a parameter
- and so on

To achieve this you can create interfaces that can be exposed, and wrap the method you want to be exposed, for an HTTP client for example.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an example we can link?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have a good example right now, I need to make that change in the java client

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I think it's good to come back here after the change. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add the example once I do the change on java yes, but the doc can still exists in the mean time

@millotp millotp requested a review from shortcuts April 26, 2022 12:24
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

Let's not forget to add the example later on. It looks good to me!

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.

4 participants