amazon-archives / amazon-cognito-js

Amazon Cognito Sync Manager for JavaScript
http://aws.amazon.com/cognito
Apache License 2.0
202 stars 83 forks source link

Fix in-memory store wipe function to check on input parameter #68

Closed dennis-tseng closed 5 years ago

dennis-tseng commented 5 years ago

Issue #, if available:

Description of changes: Fix in-memory store wipe function to check on input parameter

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

haverchuck commented 5 years ago

@dennis-tseng - Is there specific, errant behavior this PR is intended to fix? My concern is that this could change behavior for existing users of this sdk if they are expecting to catch an error when callback is undefined, as demonstrated by this code:

const testFunction = (callback) => {
  this.store = {};
  return callback(null, true)
};

const testModifiedFunction = (callback) => {
  this.store = {};
  if (callback) {
    return callback(null, true)
  }
  return this;
}

console.log('testing original function...')
try {
  const response = testFunction();
  console.log(response)
} catch (e) {
  console.log('caught an error!')
}

console.log('testing modified function...')
try {
  const response = testModifiedFunction();
  console.log(response)
} catch (e) {
  console.log('e')
}

Result:

testing original function...
caught an error!
testing modified function...
{ store: {} }

This is not to stay that the PR shouldn't be merged, necessarily, but it would be good to understand what the effects of this issue are at present.

dennis-tseng commented 5 years ago

@haverchuck The specific issue that I'm facing is that when I call wipeData through CognitoSyncManager, CognitoSyncStoreInMemory.wipe will be throwing an error stating callback is not a function.

Looks like this function is neither taking a callback nor passing a callback to wipeData AWS.CognitoSyncManager.prototype.wipeData = function () { this.provider.clearCachedId(); this.local.wipeData(); };

CognitoSyncLocalStorage.prototype.wipeData = function (callback) { this.store.wipe(callback); };

Thank you for bringing up a good point. I can catch the error on my side and assume wipeData works as intended. If that's the expectation, I'm happy to close this PR as well.

haverchuck commented 5 years ago

I think that your PR should be closed for now, but we should make this change the next time we make a breaking change to this sdk (i.e. a major version).