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

fix: need to convert instance type from the JS class to the wrapper class #1956

Merged
merged 74 commits into from
Sep 27, 2017
Merged

Conversation

wf9a5m75
Copy link
Contributor

@wf9a5m75 wf9a5m75 commented Sep 8, 2017

Please merge this pull request, and release as @ionic-native/google-maps 4.2.2.
And please specify the dependency module @ionic-native/core 4.2.0

@@ -687,21 +663,53 @@ export class BaseClass {
*
* @return {Observable<any>}
*/
@CordovaInstance({
Copy link
Collaborator

@ihadeed ihadeed Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you've removed some decorators. I suggest you replace them with CordovaCheck and InstanceCheck decorators. These two decorators check if the plugin/objectInstance exists before executing the functionality. This helps eliminate runtime errors when we're developing outside of a Cordova platform.

To use the decorators, you have to give them a configuration that specifies what to return in case Cordova is not available. You can return an empty promise, empty observable, or null.

@CordovaCheck() // returns promise
@CordovaCheck({ observable: true }) // returns observable
@CordovaCheck({ sync: true }) // returns `null`

@wf9a5m75
Copy link
Contributor Author

Thank you for your suggestion. I have been kind of busy in couple of days. I will update this later.

add: BaseClass.destroy()
add: getId() method for all instance classes
@wf9a5m75
Copy link
Contributor Author

wf9a5m75 commented Sep 17, 2017

Sorry, after the discussion with ionic team, I don't support the @ionic-native/google-maps anymore. I close this PR.

@wf9a5m75 wf9a5m75 closed this Sep 17, 2017
@wf9a5m75 wf9a5m75 reopened this Sep 21, 2017
@wf9a5m75
Copy link
Contributor Author

Since the problem has solved at once, I reopen this PR.

@wf9a5m75
Copy link
Contributor Author

@ihadeed ping

@ihadeed ihadeed merged commit 57af5c5 into danielsogl:master Sep 27, 2017
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