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: javascript sources client #97

Merged
merged 11 commits into from
Jan 21, 2022
Merged

feat: javascript sources client #97

merged 11 commits into from
Jan 21, 2022

Conversation

bodinsamuel
Copy link
Contributor

@bodinsamuel bodinsamuel commented Jan 20, 2022

🧭 What and Why

Changes included:

  • Create sources client + spec
    It was mostly because we had no way to test and it was a good opportunity to test Api Client Automation.
    Congrats to the team, it took me less than 30minutes to create a new client from the ground up 🚀

@bodinsamuel bodinsamuel self-assigned this Jan 20, 2022
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.

I was about to ask if you want to generate one 👨🏼‍🍳

@@ -146,13 +140,32 @@
"supportsES6": true,
"npmName": "@algolia/client-query-suggestions",
"npmVersion": "5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

We've kept the space to differentiate native openapi additionalProperties, and the one we manually add/use in our templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was removed by linting, you will have trouble keeping this space :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I use Save without formatting on vs code but it's a pain

Copy link
Member

Choose a reason for hiding this comment

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

Yep but it shouldn't be edited that often, not a blocker anyway, just explaining

Copy link
Member

Choose a reason for hiding this comment

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

cmd k s :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can never remember @shortcuts unlike you ahah

Copy link
Member

Choose a reason for hiding this comment

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

ahaha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never remember it and when I do cmd k s it opens the shortcut panel 😶‍🌫️

Copy link
Member

Choose a reason for hiding this comment

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

it's because it's two set of shortcuts cmd + k, trigger first mode, then you press s

Copy link
Member

Choose a reason for hiding this comment

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

You can do cmd + k then cmd + s to see the shortcuts section :D

"modelPropertyNaming": "original",
"supportsES6": true,
"npmName": "@algolia/client-sources",
"npmVersion": "0.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

I did not think of that but for recommend for example, we automatically aligned to the main algoliasearch package version. Shouldn't it also be the case for this one? Do we want to ship it in the global package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem with that, is when you need a breaking change in one (API changed) they all get a major even though nothing has changed in them. I would not align them imo

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, we need to take that in consideration when releasing, as of now we only had a generic version file for the repo but it could live in each packages

millotp
millotp previously approved these changes Jan 20, 2022
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.

We were joking about doing this this morning ! So fast

@bodinsamuel
Copy link
Contributor Author

@millotp complet chance ahah, just wanted to test our API :p

@bodinsamuel bodinsamuel marked this pull request as ready for review January 20, 2022 16:58
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 work !

shortcuts
shortcuts previously approved these changes Jan 21, 2022
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.

Looks good, only thing is what @millotp raised

Comment on lines 7 to 11
/**
* The type of the file to ingest.
*/
type: URLJobType;
input: URLJobInput;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why type have the description and input does not. Both are inlined 🤔 I wonder if it's because it's from an external file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

input has its desc on the other file, I guess it will follow along thanks to TS magic

baseHeaders: {
'content-type': 'application/x-www-form-urlencoded',
},
userAgent: 'Algolia for Javascript',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't we put the name of the client in the UA?

Copy link
Member

Choose a reason for hiding this comment

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

Not done yet, but yes we will pass the client + version

millotp
millotp previously approved these changes Jan 21, 2022
@shortcuts
Copy link
Member

Merging main and regenerating should remove the utils folder

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