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

Merge NPM package for freshdesk API with arjunkomath/node-freshdesk-api #6

Closed
maxkoryukov opened this issue Dec 26, 2016 · 9 comments
Closed

Comments

@maxkoryukov
Copy link
Contributor

Hello!

Recently we have published NPM package for Freshdesk API v2. Here is that project: https://github.com/arjunkomath/node-freshdesk-api

So, lets improve this package together in one place;)

@maxkoryukov
Copy link
Contributor Author

lust a link: maxkoryukov/node-freshdesk-api#1

@maxkoryukov
Copy link
Contributor Author

One observation: NPM allows to keep several version of the same package.

So, we could have 2 branches:

  • for API v1
  • for API v2

and appropriate NPM versions:

  • 1.x.x
  • 2.x.x

@maxkoryukov maxkoryukov changed the title Duplicate NPM package for freshdesk API * Duplicate NPM package for freshdesk API kumarharsh/node-freshdesk Dec 27, 2016
@kumarharsh
Copy link
Owner

@maxkoryukov sure, we can merge our efforts, although some work has to be done to bring this project up-to-date, such as rewriting this one in es6.

@maxkoryukov
Copy link
Contributor Author

@kumarharsh , I think, rewriting is mindless)) there are ~200 downloads per 2 NPM packages. We will spend time for rewriting the app, but we will get the same functionality...

The only one useful thing (for me), I have implemented - Error handling))

I know one thing, that could be very useful: unit- and acceptance- tests for API-client (using mock server).


Related to this ticket: I don't have a real plan how to consolidate codebases

@kumarharsh
Copy link
Owner

Maybe I can push my changes to the new node-freshdesk-api repo, and if everything goes well, I can then release the freshdesk namespace from npm so that the new combined library can be published. There's already some code in the wild which uses my version, so unpublishing from npm doesn't make sense.

@kumarharsh
Copy link
Owner

kumarharsh commented Dec 27, 2016

Also, I'd didn't see one thing in your changes @maxkoryukov - offer users a way to use the raw makeRequest function (like my library does right now) so that users don't have to wait for library maintainers to add an API route.

From the Readme:

For unimplemented routes, you can use the raw routes, such as Freshdesk.get() to make the requests directly.


Also, it depends on what the library maintainer of the other library (@arjunkomath) thinks about all this...

@kumarharsh kumarharsh changed the title * Duplicate NPM package for freshdesk API kumarharsh/node-freshdesk Merge NPM package for freshdesk API with arjunkomath/node-freshdesk-api Dec 27, 2016
@arjunkomath
Copy link

@kumarharsh I'm okay with merging. Like @maxkoryukov suggested, we can maintain both v1 and v2.

@maxkoryukov
Copy link
Contributor Author

maxkoryukov commented Dec 27, 2016

@kumarharsh

about makeRequest

For unimplemented routes, you can use the raw routes, such as Freshdesk.get() to make the requests directly.

I think, we could implement ALL Freshdesk API routes, since there are about 20-30 API calls, and I need about 1 minute to add new route to node-freshdesk-api (thanks @arjunkomath for beautiful internal design of the package).

So, I need about 30 minutes to implement all existing API functions in the package;)

On other hand, I think it is not a good practice: expose internal functions in the public API of the package (this rule holds true for almost all libraries)

About unpublishing

As described in the NPM's documentation:

It is generally considered bad behavior to remove versions of a library that others are depending on!

Consider using the deprecate command instead, if your intent is to encourage users to upgrade.

There is plenty of room on the registry.

so you shouldn't unpublish. It is enough to deprecate. And only when there will be the replacement for the package;)

@maxkoryukov
Copy link
Contributor Author

Lets move discussion to the arjunkomath/node-freshdesk-api#9

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

No branches or pull requests

3 participants