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

Historical lookup indexed db #1048

Closed
wants to merge 27 commits into from

Conversation

shandilya3
Copy link
Collaborator

@shandilya3 shandilya3 commented Oct 5, 2023

Description

IndexedDB for historical search
Wiki Doc: https://wiki.corp.adobe.com/display/Target/Historical+Events+Storage+-IndexedDB+-+In+Progress

To run locally, NPM link to this PR branch adobe/aepsdk-rulesengine-typescript#20

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA or I'm an Adobe employee.
  • I have made any necessary test changes and all tests pass.
  • I have run the Sandbox successfully.

@shandilya3 shandilya3 requested a review from jasonwaters October 5, 2023 20:10
@shandilya3 shandilya3 marked this pull request as draft October 5, 2023 20:18
@@ -59,7 +59,7 @@
}
],
"dependencies": {
"@adobe/aep-rules-engine": "^2.0.1",
"@adobe/aep-rules-engine": "file:../aepsdk-rulesengine-typescript",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for local e2e test, will add dependency once rule engine changes are approved & published.

@shandilya3 shandilya3 marked this pull request as ready for review October 9, 2023 15:19
@@ -10,15 +10,14 @@ OF ANY KIND, either express or implied. See the License for the specific languag
governing permissions and limitations under the License.
*/

const QUALIFIED_EVENT_TYPE = "decisioning.propositionQualified";
import { PropositionEventType } from "../Personalization/constants/propositionEventType";
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't allow importing between components in Alloy. If this needs to be shared between the two components, put it into src/shared.

.map(payload => payload.evaluate(context))
.filter(payload => payload.items.length > 0);
const evaluate = (context = {}) => {
return new Promise(resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The wrapping new Promise() is unnecessary and can be removed without effect. Returning something from a .then() is the same as resolving the Promise with that value.

@@ -14,10 +14,11 @@ import {
JSON_CONTENT_ITEM,
RULESET_ITEM
} from "../Personalization/constants/schema";
import { DISPLAY } from "../Personalization/constants/eventType";
import { PropositionEventType } from "../Personalization/constants/propositionEventType";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before. If it needs to be shared across components, then it should be in the shared folder.

};
});

resolve({
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before. Wrapping it in new Promise() with the resolve is unnecessary.

propositions: decisionProvider.evaluate(
contextProvider.getContext(decisionContext)
)
return new Promise(resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

createSaveStorage,
getExpirationDate
} from "./utils";
import { EVENT_TYPE_TRUE } from "../Personalization/event";
Copy link
Contributor

Choose a reason for hiding this comment

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

shared folder.

});
};

const waitForDBInitialization = fn => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Identity component, we have a similar situation: we need to wait for the identity cookie before we firing off other requests. However, the awaitIdentityCookie() is implemented with Promises.

I think if you use Promises here, it would make the implementation a little neater and removes the need for waitForDBInitialization() and it's setTimeout().

For example, if you write setupIndexDB() like so…

export default () => {
  let dbPromise;
  const setupIndexedDB = () => {
    dbPromise = new Promise((resolve, reject) => {
      // set up login
    });
    // no need to expose the DB outside of this module
    return dbPromise.then(() => {});
  };
}

Then you can use dbPromise like this…

const addRecord = record => {
  return dbPromise.then(db => {
    const transaction = db.transaction("events", "readwrite");
    const objectStore = transaction.objectStore("events");
    const objectStoreRequest = objectStore.add(record);
    return new Promise((resolve, reject) => {
      objectStoreRequest.onsuccess = () => resolve(true);
      objectStoreRequest.onerror = (txEvent) => reject(txEvent.target.error);
    });
  })
};

And you don't need the setTimeout (which is kind of a code smell, in my opinion).

return new Promise(resolve => {
decisionProvider.evaluate(context).then(propositions => {
applyResponse({ viewName, renderDecisions, propositions });
resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

new Promise() and resolve() are unnecessary.

const indexedDB = createIndexedDB();
indexedDB
.setupIndexedDB()
.then(() => console.log("IndexedDB setup is complete."));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the logger for logging messages.

);
};

request.onsuccess = event => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Identity component, we have a similar situation: we need to wait for the identity cookie before we firing off other requests. However, the awaitIdentityCookie() is implemented with Promises.

I think if you use Promises here, it would make the implementation a neater and removes the need for waitForDBInitialization() and it's setTimeout().

For example, if you write setupIndexDB() like so…

export default () => {
  /** @type {Promise<IDBDatabase>} */
  let dbPromise;
  const setupIndexedDB = () => {
    dbPromise = new Promise((resolve, reject) => {
      // set up login
    });
    // instead of returning a promise that resolves to 
    // db and exposes it outside of this module, return
    // a cleanup function
    return dbPromise.then(db => () => db.close());
  };
}

Then you can use dbPromise like this…

const addRecord = record => {
  return dbPromise.then(db => {
    const transaction = db.transaction("events", "readwrite");
    const objectStore = transaction.objectStore("events");
    const objectStoreRequest = objectStore.add(record);
    return new Promise((resolve, reject) => {
      objectStoreRequest.onsuccess = () => resolve(true);
      objectStoreRequest.onerror = (txEvent) => reject(txEvent.target.error);
    });
  })
};

And you don't need the setTimeout (which is kind of a code smell, in my opinion).

const DB_INITIALIZATION_TIMEOUT = 200;

const setupIndexedDB = () => {
return new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Identity component, we have a similar situation: we need to wait for the identity cookie before we firing off other requests. However, the awaitIdentityCookie() is implemented with Promises.

I think if you use Promises here, it would make the implementation a little neater and removes the need for waitForDBInitialization() and it's setTimeout().

For example, if you write setupIndexDB() like so…

export default () => {
  let dbPromise;
  const setupIndexedDB = () => {
    dbPromise = new Promise((resolve, reject) => {
      // set up login
    });
    // no need to expose the DB outside of this module
    return dbPromise.then(() => {});
  };
}

Then you can use dbPromise like this…

const addRecord = record => {
  return dbPromise.then(db => {
    const transaction = db.transaction("events", "readwrite");
    const objectStore = transaction.objectStore("events");
    const objectStoreRequest = objectStore.add(record);
    return new Promise((resolve, reject) => {
      objectStoreRequest.onsuccess = () => resolve(true);
      objectStoreRequest.onerror = (txEvent) => reject(txEvent.target.error);
    });
  })
};

This way, you don't need waitForDBInitialization() with it's setTimeout() or getIndexDB() (which are kind of a code smells, in my opinion).

@shandilya3
Copy link
Collaborator Author

We realized IndexedDB is more limited than expected. It's not as efficient as we had planned. Example, you can not write standard sequel statements, nor can you ask for results to be sorted (by timestamp for example). These limitations make the solution less performant than what we've already got with localStorage. With that in mind, it was decided that it would be best to stick with the existing implementation with the expectation with some changes to expect there will always be iam.id and iam.eventType in historical rulesets for IAM on the web.
We can still continue iterating on the IndexedDB approach, but we'll need to add a service worker for performance, and configuration options to the rule engine, among other things. Since we will be adding a service worker for web push, Jason recommended revisiting IndexedDB at that time.

@shandilya3 shandilya3 closed this Oct 11, 2023
@shandilya3 shandilya3 deleted the historical-lookup-indexedDB branch October 27, 2023 16:47
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.

3 participants