search

capacitor-community / background-geolocation

A Capacitor plugin that sends you geolocation updates, even while the app is in the background.
MIT License
185 stars 57 forks source link

Create typescript definitions #3

Closed pdrhlik closed 3 years ago

pdrhlik commented 3 years ago

This PR creates typescript definitions and adds simple documentation to each function and property. It also modifies a README a bit to reflect the changes, though not extensively yet.

The naming conventions were inspired by https://developer.mozilla.org/en-US/docs/Web/API/Geolocation_API so it stays as close as possible.

Closes #2

pdrhlik commented 3 years ago

The only thing that is not typed now is the Geolocation error. What is the structure of that error, @diachedelic? Does it return anything else than a code property with a NOT_AUTHORIZED value or not for now?

I would fix that right away, thanks.

diachedelic commented 3 years ago

I have some comments:

  1. Why does this configure the Typescript compiler? Is this required for typings?
  2. Why does it now expose an unimplemented Web plugin?
  3. It is confusing to give the single object parameter a name, can it be anonymous, or just value? removeWatcher(id: {id: string})
  4. Some fields on the location object are optional. Can this be part of the typescript definition?
  5. The project uses JSLint for javascript, which is incompatible with Prettier. Also, I have come to dislike how Prettier formats javascript.
  6. In the README, I would prefer to show the Typescript usage separately from the JS usage. This is because everyone uses JS, but not everyone uses Typescript.
  7. addWatcher does not return a Promise.

Sorry, actually I am not going to merge this. The changes introduce too much complexity and repetition. I was imagining it would simply be the addition of a single file containing type definitions.

pdrhlik commented 3 years ago

I followed the official instructions for creating Capacitor plugins and all this complexity seems to be necessary. I put back all the files that where missing in this repo when you create a plugin (v2) using npx @capacitor/cli plugin:generate.

  1. It's probably not required but I would call it good practice. It checks and validates the definitions for you. It's only a devDependency.
  2. I had some issues making it work without it. I would add a simple web implementation in the future.
  3. When I only passed a value to addWatcher(), it didn't work. Is it possible that it's because of this line? String callbackId = call.getString("id"); in BackgroundGeolocation.java#L158?
  4. Optional fields are not a problem at all. It seems that only latitude, longitude and time are always present. Is that right?
  5. Again, I used the official boilerplate. I think prettier doesn't have to be a part of this.
  6. This makes sense.
  7. That will be easy to fix.

I hope you will reconsider because every official and non-official @capacitor-community plugin have typescript definitions and they do it exactly like this.

I think that this plugin has a great potential and I would like to help make it better. Otherwise I'll just tend to my forked version.

diachedelic commented 3 years ago

Thanks for your work on this. I have slightly modified the definitions you wrote and release them in v0.3.7, please let me know if they are not satisfactory.