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

Describe status constants possible values #371

Merged
merged 1 commit into from
Sep 30, 2019
Merged

Conversation

fcamblor
Copy link

@fcamblor fcamblor commented Sep 24, 2019

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation changes
  • Other... Please describe:

What is the purpose of this PR?

Today, for example, we are not able to use motionStatus constants in a typesafe manner (there is no check on the existence of this or that motion status since the type is any)

This PR aims at fixing this.

Does this PR introduce a breaking change?

  • Yes
  • No

Conceptually, yes, this is a breaking change in terms of type descriptor (today's any has a wider type space than the one I'm proposing)

However, this is for good : people using window.cordova.plugins.diagnostic.motionStatus.NOT_DETERMINNED (typo in the name) in their code were not having any error, and now they will have some.

Note that it may help pinpoint migration issues introduced recently (like #230) in case people doesn't read the changelog :-)

@fcamblor
Copy link
Author

⚠️ Please wait, I found some inconsistencies between android and ios constant values, will push the fix quickly

@fcamblor
Copy link
Author

should be ready for review now :-)

Allows to reference those constants in a typesafe manner
@fcamblor
Copy link
Author

Also, note that I prepared a branch, on my local repo, targetting version 4.x : https://github.com/fcamblor/cordova-diagnostic-plugin/tree/ts-4x-constants

I cannot propose any PR yet for 4.x as there is no real branch targetting this branch currently on your repository :)
If you're interested by such PR, please create a dedicated 4.x branch and I'll submit it :-)

@dpa99c
Copy link
Owner

dpa99c commented Sep 30, 2019

Great, thanks for this.

@dpa99c dpa99c merged commit d69316d into dpa99c:master Sep 30, 2019
@dpa99c
Copy link
Owner

dpa99c commented Sep 30, 2019

This has been published in v5.0.1

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.

2 participants