Replies: 14 comments
-
Hello,
Thank you for reaching Crowdin team!
Kindly note that our replies may be slower than usual due to national
holidays until January 8th (response time should not exceed 24h though),
also Live Chat will become active again from January 8th, 2022.
Any urgent inquiries will be resolved with the higher priority in the
queue, inbox is occasionally monitored 👍
All the best in 2022!
…--
Sincerely yours,
Crowdin Team
{#HS:1749665491-180430#}
|
Beta Was this translation helpful? Give feedback.
-
Hi @ImRodry, Thanks a lot for the detailed suggestion! Regarding helper methods - the library already provides the solution for fetching all items in a list instead of creating loops on a client-side: ...
// get all projects
projectsGroupsApi
.withFetchAll()
.listProjects()
.then(projects => console.log(projects))
.catch(error => console.error(error));
// get projects but not more than 1000
projectsGroupsApi
.withFetchAll(1000)
.listProjects()
.then(projects => console.log(projects))
.catch(error => console.error(error)); Regarding providing much more developer-friendly objects - it will lead to a huge rewriting of a library, and as a result - the complication of the library due to the creation of a large number of response models (and possibly additional request models). Our goal during development was to keep the library clean and simple. @yevheniyJ what do you think about it? |
Beta Was this translation helpful? Give feedback.
-
Oh alright that's my bad, didn't know that
The point of this issue is exactly to propose a huge rewrite of the library. At the end of the day, that's what semver major releases are for. My only proposal for the object types is for them to be split up into Crowdin and Crowdin Enterprise and for the inner and outer data fields to be removed, which would simplify the objects and types, not complicate them or increase them
Keeping a library simple to use is good of course, but simple in the number of features is not, if you know what I mean. Having these split clients and some form of caching of retrieved data would help a lot, even though Crowdin does not offer a gateway with incoming events, it would help in certain situations. |
Beta Was this translation helpful? Give feedback.
-
Hi @ImRodry! Thanks for the details! I will pass everything to the team and come back to you with the updates |
Beta Was this translation helpful? Give feedback.
-
I think this proposal can also be split into two PRs if accepted:
So once you hear back from the team let me know and I can start work on those if you so wish :) |
Beta Was this translation helpful? Give feedback.
-
Sure, as soon as we have the updates, we'll reach you immediately :) |
Beta Was this translation helpful? Give feedback.
-
Hi hi! Let me bring some light to this topic. Initially this library was designed to support Enterpirse version of the API as is. Without any intermediate logic. Later on we also had to add support of the API v2. And actually v2 is just a subset of functions of the Enterpirse API. But yep, there are still some differences in the types which leads to this non-ideal library design. But here I am not sure if separating them into 2 different client classes for each API version will be safe. We have quite a lot of places where this library is used in version-agnostic context. Which means that we can work either with API v2 or with Enterpirse at the same time. And because we have one type for both it then does not cause any issues. So maybe it will also end up with having a parent class that will hold all shared methods. So users can either work with a concrete version-based class or with the parent abstract class. In any case this refactoring then needs to be properly organized and prioritized. |
Beta Was this translation helpful? Give feedback.
-
I feel like that package is more for OAuth2 apps and not just pure Crowdin API, or at least that's what it seems like at first glance.
That is why I suggested having a BaseCrowdinClient class that houses all methods that have common input AND output between the two API versions. The other version-specific methods could be placed on the main classes.
That's what I'm here for! I proposed splitting this into two PRs above: a semver minor one followed by a semver major, so please let me know how that sounds and if you'd like to proceed with this! |
Beta Was this translation helpful? Give feedback.
-
String enums are merged and I suggest pausing this overhaul for now and do not rush to split the API Client. |
Beta Was this translation helpful? Give feedback.
-
I’m not rushing it, but with that PR merged the next version release will have to be v2.0.0 if it includes that commit so I believe it would be nice to do all of that so that the repo doesn’t get stuck |
Beta Was this translation helpful? Give feedback.
-
Discussed it with @yevheniyJ and we are agreed that it will be |
Beta Was this translation helpful? Give feedback.
-
But the current version is 1.15.2 ? Why that specifically and why isn’t this a breaking change? |
Beta Was this translation helpful? Give feedback.
-
These enums are used not so often by users. This is not enough for such a large version increment, IMO. Additionally, we will add a notice to the release notes. As I said before, we don't want to support both versions and will do everything possible to keep a single version of the API Client. |
Beta Was this translation helpful? Give feedback.
-
Also quite a lot of clients are using library with vanilla JS where there will be no difference |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Hey! I'm making this issue as a way to propose a big change to the library as a whole. The issue will be split into different sections explaining my motivations for this, as well as possible solutions I've considered
So, what's wrong?
Unsafe types
Currently, the library mixes regular Crowdin API endpoints with Crowdin Enterprise API endpoints and, while some are shared, Enterprise, unfortunately, has a lot of endpoints that regular Crowdin does not and in some common endpoints, the responses are different, which has led to issues in types like the one solved in #132. These factors lead to a very type-unsafe library, which is not good considering the library is even built in TypeScript natively.
Simplicity of the library
Another issue I've faced while using the package is that it is very simple, in the sense that it doesn't offer any helper functions or data transformation, and it simply gives you 1 method for each endpoint and returns the exact data that the API returns. In order to better explain this point, I'll divide it into two sections
Transforming API data
While it's nice that you give us the raw data received from the API call, I don't think this is enough. This could be improved by creating classes that house methods to manage specific structures instead of having to call the endpoint and pass the arguments ourselves. A very good example of this is how the
discord.js
package wraps around the Discord API providing managers, classes for every Discord structure, and methods to edit and manage them directly, without the developer ever having to access the API directly. Without this, the library simply feels like a slightly customized version of axios, and there's no real advantage to using it instead of simply crafting our requests ourselves using the aforementioned package.Helper methods
As mentioned previously, the library lacks helper methods for certain parts of the API. For example, the API has a hard limit of 500 structures that can be returned per call, but sometimes we need to obtain more than 500 structures, so we need multiple API calls to achieve this. This results in us developers having to make functions like this.
In this function, I am simply getting all the strings in a project, but I have to create this while loop in order to achieve that, something that could easily be done on the library's side.
Possible solution
As described earlier, an example of an API wrapper that I personally really like is discord.js which achieves all of the points mentioned above, however, Crowdin is different in some ways so allow me to summarize the ideas I had for the library overall:
data
property and provide a much more developer-friendly object, while still allowing access to the raw data through a property on the class.I would love to contribute to a possible rewrite of the library including the features mentioned here, so I'd like to hear your feedback, both from the maintainers and the community that uses this package so that we can reach an agreement and improve the library. Thank you for reading through!
Beta Was this translation helpful? Give feedback.
All reactions