EddyVerbruggen / nativescript-secure-storage

:closed_lock_with_key: NativeScript plugin for secure local storage of fi. passwords
MIT License
111 stars 26 forks source link

First run should not be assigned on constructor #35

Closed kefahB closed 4 years ago

kefahB commented 4 years ago

Hi @EddyVerbruggen, I remark an issue on the concept to do stuff with "First run", you declare this on constructor :

  constructor() {
    this.isFirst = applicationSettings.getBoolean(SecureStorageCommon.IS_FIRST_RUN, true);
    if (this.isFirst) {
      applicationSettings.setBoolean(SecureStorageCommon.IS_FIRST_RUN, false);
    }
  }

Since this is declared on constructor we can not do some think with the first run because you set immediately the SecureStorageCommon.IS_FIRST_RUN to false, if I call the isFirstRun() or clearAllOnFirstRun() or clearAllOnFirstRunSync() will return always false cause t it is already set on the constructor.

Maybe change refactor the tow method isFirstRunSync and isFirstRunSync and delete those lines from constructor :

public isFirstRunSync(): boolean {
     if (this.isFirst) {
          applicationSettings.setBoolean(SecureStorageCommon.IS_FIRST_RUN, false);
          return true;
    }

   return false;
}

public isFirstRun(): Promise<boolean> {
    return new Promise<boolean>((resolve, reject) => {
         if (this.isFirst) {
            applicationSettings.setBoolean(SecureStorageCommon.IS_FIRST_RUN, false);
            resolve(true);
         }
        resolve(false);
    });
}
EddyVerbruggen commented 4 years ago

@kefahB Actually, the value is cached in this.isFirst (second line in your first snippet). So as long as that instance is reused this line will return that initially cached value.

kefahB commented 4 years ago

@EddyVerbruggen, Yes of course, but the I think is that the issu in it, is cached to false before that we can use the functions isFirstRun or isFirstRunSync.

Maybe I am not clean how explain, I give an exemple :

index.ts

// Here first run is TRUE
const secure = new SecureStorage();

// Now first run is FALSE

secure.isFirstRun.then(res => {

    console.log(res); // It always will be false because is cached to false on constructor

    if(res) {
        // Do some stuff....
    }

});

If I have to do some stuff when the value is true I can not because it will never be true after caller new SecureStorage()

EddyVerbruggen commented 4 years ago

@kefahB If that cached value is false then there's a bug because it's supposed to be true on first run and secure.isFirstRun should return that value.

Do you see this issue on both platforms?

kefahB commented 4 years ago

No just on IOS cause.

I will do some extra testing and tell you .. I know you have a lot to do 😉