search

ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 208 forks source link

feat: add TypeScript support #359

Closed p-kuen closed 2 weeks ago

p-kuen commented 2 years ago

I managed to convert the package into typescript. The change should not cause any breaking changes as all tests are the same as before and were successful.

I want to mention a few points: The code includes a lot of input-checks which allows to input lots of undefined or unwanted data. I recommend to change the strategy a little bit. We should expect from the user of this package to use the correct api. If it is not used correctly, it should not be the problem of the package which should handle every possible case. In my opinion this leads to huge fallback and help code which a good developer does not need. I added those fallback and helper code in this typescript refactor, but strongly recommend to remove it. Especially the type system helps a lot to use the correct api.

Another change was to replace the whole lodash package with the lodash.defaultsdeep package to reduce dependency size.

I also strongly recommend to get rid of the request package as it is deprecated since about two years (https://www.npmjs.com/package/request)

I would be happy if this PR gets merged. If it does not apply to the strategy of the package owner, I would publish an alternative package to npm with those and some additional changes (with clear mention and reference to this package of course)

Feedback is very welcome!

TheVaan commented 1 year ago

Hey @p-kuen, seems like this package won't get updated/migrated to TS. Please move this to an alternative package including the mentioned points that could be improved. I would really appreciate this!

mtrezza commented 1 year ago

This package is still under maintenance, if you mean that, @TheVaan. This just needs a review. Anyone is free to review, but I haven't seen any feedback yet from the community.

TheVaan commented 1 year ago

What I meant is, that there is no feedback from the maintainer / owner. This can be interpreted as there is no wish to switch the codebase and fix the mentioned points (or at least not having time to work on this PR). These points (and the switch to typescript) are (in my opinion) nothing the community can decide. Switching to typescript is definitely something the owner or the main contributors have to decide.

p-kuen commented 1 year ago

@TheVaan Thank you for your efforts. I also think that it should be considered to merge these changes or at least start a discussion.

I really do not want to publish a new package as it is not my vision of Open Source projects, but if nobody wants to change something (bigger), there is no other possibility..

mtrezza commented 1 year ago

It seems definitely a step forward and I can't see an argument against Typescript. It's may just be a huge and complex effort depending on the size of the codebase. I think it's definitely more a feat than a refactor, maybe it even deserves a major release.

When switching to TypeScript, moving to a new major version and dropping deprecated functions etc should be considered to reduce unnecessary code.

Anything in particular that caught your eye?

TheVaan commented 1 year ago

Anything in particular that caught your eye?

https://github.com/ToothlessGear/node-gcm/blob/master/lib/result.js https://github.com/ToothlessGear/node-gcm/blob/master/lib/multicastresult.js and several as deprecated marked constants.

Additionally I would recommend switching to Promise / async/await architecture when going away from deprecated request package. Writing callbacks feels a little bit historical, especially when working with (maybe) heavy thread blocking tasks (see retry).

mtrezza commented 1 year ago

I agree with your points; who would want to pick this up (in a separate PR)? Maybe you could open an issue for each of these changes so we track and distribute them easier.

TheVaan commented 1 year ago

I would work on some of these points, but I'm waiting for TypeScript, because of type safety and because @types/node-gcm would get incompatible if we change stuff now.

mtrezza commented 1 year ago

@p-kuen Could you rebase this PR on master, I've added CI tests.

@ToothlessGear We should change the repo settings to not allow to merge a PR that is not on the laster master commit. This PR isn't but I see the "Squash and merge" button.

mtrezza commented 1 year ago

Can we come up with a changelog entry in the meantime? Like:

BREAKING CHANGE

This release migrates the package to TypeScript and refactors the files and code structure internally. It's therefore released as a new major version.

Features

  • Add TypeScript support
p-kuen commented 1 year ago

I just rebased the branch to be updated with the upstream branch.

mtrezza commented 1 year ago

Started the CI to see whether it passes

p-kuen commented 1 year ago

I just recreated the package.lock with npm v6 to get the lockfile version 1. In the npm docs it says that the lockfile version is backwards compatible (https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion). Therefore I see no problem in using that lockfile version.

p-kuen commented 1 year ago

What's the current status on this PR?

p-kuen commented 10 months ago

I just rebased the code and I still see no problems in merging this PR. The last release of this package is 2 years ago, an update would be a good sign that this package is still maintained.

We could integrate this PR in a new major upgrade and I would be glad to contribute for further optimizations mentioned in the last few comments.

p-kuen commented 2 weeks ago

I will finally close this PR as this package will be deprecated (29fe4973e06d5780afee5e53c2bc90a850864378)