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

docs: add migration guide page #449

Merged
merged 6 commits into from
Apr 29, 2022
Merged

docs: add migration guide page #449

merged 6 commits into from
Apr 29, 2022

Conversation

shortcuts
Copy link
Member

🧭 What and Why

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

Changes included:

Adds the migration guide page to list the breaking changes and some more snippets.

It's not as exhaustive as the Algolia doc, but it should come together after the most frequent use cases are listed.

🧪 Test

Netlify preview :D

@shortcuts shortcuts requested a review from a team April 28, 2022 13:05
@shortcuts shortcuts self-assigned this Apr 28, 2022
@shortcuts shortcuts requested review from eunjae-lee and millotp and removed request for a team April 28, 2022 13:05
@netlify
Copy link

netlify bot commented Apr 28, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit f5c1e2e
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/626bac288471090008a01a28
😎 Deploy Preview https://deploy-preview-449--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.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 28, 2022

✗ The generated branch has been deleted.

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

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.

Very informative !

To get started, you first need to install `algoliasearch` (or any other available API client package). You can find the full list of available packages [here](https://www.npmjs.com/org/experimental-api-clients-automation).

All of our clients comes with type definition, and are available for both `browser` and `node` environments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As our docs become bigger, is there a way to split those files per languages ? It's not urgent at all

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep it would make us duplicate content, however I think we could read the snippets from an external file and inject it here, since it's JS

Copy link
Member Author

@shortcuts shortcuts Apr 28, 2022

Choose a reason for hiding this comment

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

Since we know in the end that it will most likely live in the official doc I don't think we should dedicate time for that but it's definitely doable

website/docs/api-clients/introduction.md Outdated Show resolved Hide resolved
website/docs/api-clients/migration-guide.mdx Outdated Show resolved Hide resolved
website/docs/api-clients/migration-guide.mdx Outdated Show resolved Hide resolved
website/docs/api-clients/migration-guide.mdx Outdated Show resolved Hide resolved
@shortcuts shortcuts requested a review from millotp April 29, 2022 08:20
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 good ! Code you also remove the font for codebolocks in dark mode ?

@shortcuts
Copy link
Member Author

Looking good ! Code you also remove the font for codebolocks in dark mode ?

@millotp If you check https://deploy-preview-449--api-clients-automation.netlify.app/docs/api-clients/installation/ it's also removed for dark mode, isn't it for you?

@millotp
Copy link
Collaborator

millotp commented Apr 29, 2022

In dark mode it's still in italic

@shortcuts
Copy link
Member Author

In dark mode it's still in italic

good after f5c1e2e

@shortcuts shortcuts requested a review from millotp April 29, 2022 09:25
@shortcuts shortcuts merged commit 00fbcde into main Apr 29, 2022
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.

massive work! 🎉


The amount of changes in this new version is significant. If you were using a version older than v4, please also read [this migration guide](https://www.algolia.com/doc/api-client/getting-started/upgrade-guides/javascript/)

**You should thoroughly test your application once the migration is over.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need this line. It's kind of obvious. Also, it's not that we're providing any special tool or guide to test things 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep I agree it's not that mandatory, but I feel like it's still good to mention it so we don't get people saying their prod is broken after upgrade :D


| Previous | Latest | Description |
| ----------- | :---------- | :------------------------------------------------- |
| `initIndex` | **removed** | All methods are now available at the client level. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any example to work around this somewhere else? If not, we should add an example here, to actually explain what it means to them, and what they need to do to migrate.

edit: I just found "Methods targetting an indexName" section below. Should we add a link here to that section? Or just a very short example to give a brief idea?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep indeed I'll link it!

Comment on lines +96 to +100
- import algoliasearch from 'algoliasearch/lite';
+ import { algoliasearchLiteClient } from '@experimental-api-clients-automation/algoliasearch-lite';

- import algoliasearch from 'algoliasearch';
+ import { algoliasearch } from '@experimental-api-clients-automation/algoliasearch';
Copy link
Contributor

Choose a reason for hiding this comment

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

Very readable with diff!

Btw, it's a side note, but it's a bit inconsistent to me that we have algoliasearch and algoliasearchLiteClient. The latter could simply be algoliasearchLite. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep definitely, the only thing is that since it's generated, it adds the Client automatically.

So either we make algoliasearch consistent, either we remove the Client suffix (for JS only?) I don't really like the latter

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, I don't like the latter either! Let's leave it as-is, and think about it later :)

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