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(secure-storage): reject when the plugin doesn't exist #1562

Merged
merged 1 commit into from
May 17, 2017

Conversation

eboukamza
Copy link
Contributor

The create() method reject the promise when the plugin is not available for avoid return a never fulfilled promise when we call the methods of SecureStorageObject

Closes #1515

@ihadeed
Copy link
Collaborator

ihadeed commented May 16, 2017

This behavior is intentional for plugins that create an instance of another object. In this situation the promise should ideally reject but in other cases like Google Maps it's better to create an instance object that doesn't do anything. Here's an example of why this is ideal:

let map: GoogleMap = this.googleMaps.create(...);

// the browser will throw an error because map.on doesn't exist
map.on('MAP_READY')
  .then(() => { ... });

I will review these plugins again and implement a universal fix. Maybe the plugins that return a promise (like this one) should reject, and the rest of them can return a dummy object.

@ptitjes
Copy link

ptitjes commented May 16, 2017

@ihadeed The problem is that the user cannot see any difference between a dummy object and a real object. So the promise should resolve to null or be rejected.

For instance, you can't do things like the following because the navigation will never be set :

    this.platform.ready()
      .then(() => this.secureStorage.create(SECURE_STORAGE_NAME))
      .then(storage => storage ? storage.get(SECURE_STORAGE_KEY) : null)
      .then(credentials => credentials ? this.auth.login(credentials) : false)
      .then(granted => granted ? this.nav.setRoot(MainPage) : this.nav.setRoot(LoginPage))
      .catch(error => this.nav.setRoot(LoginPage))

Would you mind to accept the patch from @eboukamza, please ?

@eboukamza
Copy link
Contributor Author

eboukamza commented May 17, 2017

@ihadeed
I agree that is a quick fix but in my case this helps to turn my app in dev mode (browser).

If you want to work in a more elegant solution I can help you too.

@@ -125,7 +125,7 @@ export class SecureStorage extends IonicNativePlugin {
if (checkAvailability('cordova.plugins.SecureStorage', null, 'SecureStorage') === true) {
const instance = new cordova.plugins.SecureStorage(() => res(new SecureStorageObject(instance)), rej, store);
} else {
res(new SecureStorageObject());
rej('SecureStorage failure: cordova plugin is not available');
Copy link
Collaborator

@ihadeed ihadeed May 17, 2017

Choose a reason for hiding this comment

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

Replace this block of code with: Never mind this comment, read the next comment below

const availabilityCheck: any = checkAvailability(SecureStorage.getPluginRef(), null, SecureStorage.getPluginName());
if (availabilityCheck === true) {
  const instance = new cordova.plugins.SecureStorage(...); // same code as above
} else {
  rej(availabilityCheck.error);
}

Copy link
Collaborator

@ihadeed ihadeed May 17, 2017

Choose a reason for hiding this comment

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

Sorry forget about the comment above, here's a better way to do it:

@CordovaCheck()
create(): Promise<SecureStorageObject> {
  return new Promise<SecureStorageObject>((resolve: Function) => {
    const instance = new cordova.plugins.SecureStorage(...);
  });
}

The @CordovaCheck() decorator will make it only run if the checkAvailability passes, and it will automatically reject with the error if it exists.

@ihadeed ihadeed merged commit d5919d1 into danielsogl:master May 17, 2017
@eboukamza eboukamza deleted the secure-storage-fix branch May 17, 2017 12:07
@eboukamza
Copy link
Contributor Author

@ihadeed you are a unsolved conflict with the commit dfd0514 in the merge 7d63e80

@ihadeed
Copy link
Collaborator

ihadeed commented May 17, 2017

@eboukamza thanks I fixed it and published a new release now. I didn't notice that until I tried building the plugin.

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.

None yet

3 participants