From a5766e5367c69bbfe0a0d48d5eb9816940a611b6 Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Thu, 20 Aug 2020 17:22:37 -0700
Subject: [PATCH 1/7] Added comments

---
 Makefile                 | 12 +++++-----
 scripts/version.js       |  1 +
 src/amplitude-client.js  |  7 ++++--
 src/amplitude-snippet.js |  6 +++++
 src/amplitude.js         |  2 +-
 src/index.js             |  7 ++++++
 src/metadata-storage.js  | 21 ++++++++++++-----
 src/notes.md             | 51 ++++++++++++++++++++++++++++++++++++++++
 src/options.js           |  2 +-
 src/top-domain.js        |  1 +
 10 files changed, 94 insertions(+), 16 deletions(-)
 create mode 100644 src/notes.md

diff --git a/Makefile b/Makefile
index 400c6e0f..050250f7 100644
--- a/Makefile
+++ b/Makefile
@@ -66,14 +66,14 @@ README.md: $(SNIPPET_OUT) version
 
 $(OUT): node_modules $(SRC) package.json rollup.config.js rollup.min.js rollup.native.js rollup.esm.js rollup.umd.js rollup.umd.min.js
 	@$(JSHINT) --verbose $(SRC)
-	@NODE_ENV=production $(ROLLUP) --config rollup.config.js
-	@NODE_ENV=production $(ROLLUP) --config rollup.esm.js
-	@NODE_ENV=production $(ROLLUP) --config rollup.umd.js
-	@NODE_ENV=production $(ROLLUP) --config rollup.native.js
-	@NODE_ENV=production $(ROLLUP) --config rollup.nocompat.js
+	@NODE_ENV=production $(ROLLUP) --config rollup.config.js # is the snippet build config
+	@NODE_ENV=production $(ROLLUP) --config rollup.esm.js # does not concat dependencies, only has module and dependencies 
+	@NODE_ENV=production $(ROLLUP) --config rollup.umd.js # generates npm version, also usable in require js app
+	@NODE_ENV=production $(ROLLUP) --config rollup.native.js # generates react native build
+	@NODE_ENV=production $(ROLLUP) --config rollup.nocompat.js # may be able to remove
 	@NODE_ENV=production $(ROLLUP) --config rollup.umd.min.js
 	@NODE_ENV=production $(ROLLUP) --config rollup.min.js
-	@NODE_ENV=production $(ROLLUP) --config rollup.nocompat.min.js
+	@NODE_ENV=production $(ROLLUP) --config rollup.nocompat.min.js # may be able to remove
 
 #
 # Target for minified `amplitude-snippet.js` file.
diff --git a/scripts/version.js b/scripts/version.js
index a80d64c0..2df50db3 100644
--- a/scripts/version.js
+++ b/scripts/version.js
@@ -30,6 +30,7 @@ replaceTextInFile(
 );
 
 // Update integrity hash in snippet
+// Provides extra layer of security. If script changes, it will fail to load
 const sdkText = fs.readFileSync(path.join('.', `amplitude.min.js`), 'utf-8');
 const hash = crypto.createHash('sha384').update(sdkText).digest('base64');
 replaceTextInFile(
diff --git a/src/amplitude-client.js b/src/amplitude-client.js
index b782ec0a..2f4cb3c8 100644
--- a/src/amplitude-client.js
+++ b/src/amplitude-client.js
@@ -1,14 +1,15 @@
+// Core of SDK code
 import Constants from './constants';
 import cookieStorage from './cookiestorage';
 import MetadataStorage from '../src/metadata-storage';
-import getUtmData from './utm';
+import getUtmData from './utm'; // Urchin Tracking Module 
 import Identify from './identify';
 import localStorage from './localstorage';  // jshint ignore:line
 import md5 from 'blueimp-md5';
 import Request from './xhr';
 import Revenue from './revenue';
 import type from './type';
-import UAParser from '@amplitude/ua-parser-js';
+import UAParser from '@amplitude/ua-parser-js'; // Identifying device and browser info (maybe move to backend?)
 import utils from './utils';
 import UUID from './uuid';
 import base64Id from './base64Id';
@@ -51,6 +52,7 @@ var AmplitudeClient = function AmplitudeClient(instanceName) {
   this._identifyId = 0;
   this._lastEventTime = null;
   this._newSession = false;
+  // sequence used for by frontend for prioritizing event send retries
   this._sequenceNumber = 0;
   this._sessionId = null;
   this._isInitialized = false;
@@ -1567,6 +1569,7 @@ AmplitudeClient.prototype.sendEvents = function sendEvents() {
 
 /**
  * Merge unsent events and identifys together in sequential order based on their sequence number, for uploading.
+ * Identifys given higher priority than Events. Also earlier sequence given priority
  * @private
  */
 AmplitudeClient.prototype._mergeEventsAndIdentifys = function _mergeEventsAndIdentifys(numEvents) {
diff --git a/src/amplitude-snippet.js b/src/amplitude-snippet.js
index d4ff393d..1a54175d 100644
--- a/src/amplitude-snippet.js
+++ b/src/amplitude-snippet.js
@@ -1,3 +1,9 @@
+/**
+* Imported in client browser via <script> tag
+* Async capabilities: Interally creates stubbed window.amplitude object until real SDK loaded
+* Stubbed functions keep track of funciton calls and their arguments
+* These are sent once real SDK loaded through another <script> tag
+*/
 (function(window, document) {
   var amplitude = window.amplitude || {'_q':[],'_iq':{}};
   var as = document.createElement('script');
diff --git a/src/amplitude.js b/src/amplitude.js
index 028bbfff..306ebfaf 100644
--- a/src/amplitude.js
+++ b/src/amplitude.js
@@ -8,7 +8,7 @@ import { version } from '../package.json';
 import DEFAULT_OPTIONS from './options';
 
 /**
- * Amplitude SDK API - instance manager.
+ * Legacy API of Amplitude SDK - instance manager. Wraps around the current amplitude-client.js which provides more features
  * Function calls directly on amplitude have been deprecated. Please call methods on the default shared instance: amplitude.getInstance() instead.
  * See [Readme]{@link https://github.com/amplitude/Amplitude-Javascript#300-update-and-logging-events-to-multiple-amplitude-apps} for more information about this change.
  * @constructor Amplitude
diff --git a/src/index.js b/src/index.js
index d975c26a..b3a626ea 100644
--- a/src/index.js
+++ b/src/index.js
@@ -1,15 +1,22 @@
 /* jshint expr:true */
+// Entry point
 import Amplitude from  './amplitude';
 
 const old = window.amplitude || {};
 const newInstance = new Amplitude();
 newInstance._q = old._q || [];
+
+/**
+* Instantiates Amplitude object and runs all queued function logged by stubbed methods provided by snippets
+* Event queue allows async loading of SDK to not blocking client's app
+*/
 for (let instance in old._iq) { // migrate each instance's queue
   if (old._iq.hasOwnProperty(instance)) {
     newInstance.getInstance(instance)._q = old._iq[instance]._q || [];
   }
 }
 
+// If SDK is enabled as snippet, process the events queued by stubbed function
 if (BUILD_COMPAT_SNIPPET) {
   newInstance.runQueuedFunctions();
 }
diff --git a/src/metadata-storage.js b/src/metadata-storage.js
index 7c4d7d1a..f78ede01 100644
--- a/src/metadata-storage.js
+++ b/src/metadata-storage.js
@@ -9,7 +9,13 @@ import getLocation from './get-location';
 import localStorage from './localstorage'; // jshint ignore:line
 import topDomain from './top-domain';
 
-class MetadataStorage {
+/**
+* MetadataStorage involves SDK data persistance
+* storage priority: cookies -> localStorage -> in memory 
+* if in localStorage, unable track users between subdomains
+* if in memory, then memory can't be shared between different tabs
+*/
+ class MetadataStorage {
   constructor({storageKey, disableCookies, domain, secure, sameSite, expirationDays}) {
     this.storageKey = storageKey;
     this.disableCookieStorage = !baseCookie.areCookiesEnabled() || disableCookies;
@@ -35,18 +41,21 @@ class MetadataStorage {
     return `${this.storageKey}${suffix ? `_${suffix}` : ''}`;
   }
 
+  /*
+  * Data is saved as delimited values rather than JSO to minimize cookie space
+  * Should not change order of the items
+  */
   save({ deviceId, userId, optOut, sessionId, lastEventTime, eventId, identifyId, sequenceNumber }) {
-    // do not change the order of these items
     const value = [
       deviceId,
-      Base64.encode(userId || ''),
+      Base64.encode(userId || ''), // used to convert not unicode to alphanumeric since cookies only use alphanumeric
       optOut ? '1' : '',
-      sessionId ? sessionId.toString(32) : '0',
-      lastEventTime ? lastEventTime.toString(32) : '0',
+      sessionId ? sessionId.toString(32) : '0', // generated when instantiated, timestamp (but re-uses session id in cookie if not expired) @TODO clients may want custom session id
+      lastEventTime ? lastEventTime.toString(32) : '0', // last time an event was set
       eventId ? eventId.toString(32) : '0',
       identifyId ? identifyId.toString(32) : '0',
       sequenceNumber ? sequenceNumber.toString(32) : '0'
-    ].join('.');
+    ].join('.'); 
 
     if (this.disableCookieStorage) {
       localStorage.setItem(this.storageKey, value);
diff --git a/src/notes.md b/src/notes.md
new file mode 100644
index 00000000..c9df11e2
--- /dev/null
+++ b/src/notes.md
@@ -0,0 +1,51 @@
+# Intro
+- Three ways for SDK to be loaded
+  - Standard NPM package
+  - Snippet in `<script>` tag
+  - RequireJS (may be dropped in future)
+- Build system is Make instead of yarn (legacy, may possibly be updated)
+- Use Amplitude Instrumentation Explorer to help development
+  - "Instrumentation" involves matters related to logging user actions (i.e. events)
+
+# Concerns
+- Keep in mind backwards compatibility
+- Keep SDK build size as small as possible
+- Should support as many browsers as possible
+
+# Architecture
+- `index.js` is the main entrypoint of SDK
+- Stubbed methods are used when client imports via `<script>` snippet
+  - Allows app to not be blocked while real JS SDK is loaded in
+  - Sent events and identifys are tracked with queues
+- Metadata storage (new) vs cookie (old) storage
+  - more of issue with anonymous id, because it uses device id instead of user id
+- UA Parser: Helps identify browsers
+  - might be able to use upstream library and convert results, rather than our fork
+- sameSiteCookie: Sets how public the cookie reading is
+  - `amplitude.getInstance() is necessary even during reuse because of snippet stubbed
+    - only applicable to snippet import
+    - Can possibly do better?
+
+# Development
+- Run `yarn dev` and open `localhost:9000` for development tools in browser
+
+# Misc
+- Readme.io used for documentation
+
+# Ideas
+- E2E browser tests with sauce labs
+- merge with node?
+
+Question
+- Browser compat?
+- MaxMind
+  - Determines region from IP address
+- Some custoemrs wont update SDK
+- Consideration to bring up durign PE weekly monday discussion
+  - browserslist?
+  - eslint plugin compat?
+  - Automation for updating docs when new SDK version is published
+  - Google Tag Manager: Allows people at runtime to add random script tag
+- No plans to stop snippet
+  - might stop require js
+- user & account lookup, allows seeing stream of events sent from users
\ No newline at end of file
diff --git a/src/options.js b/src/options.js
index 5f3f75f7..0b16980d 100644
--- a/src/options.js
+++ b/src/options.js
@@ -16,7 +16,7 @@ export default {
   batchEvents: false,
   cookieExpiration: 365 * 10,
   cookieName: 'amplitude_id', // this is a deprecated option
-  sameSiteCookie: 'Lax',
+  sameSiteCookie: 'Lax', // cookie privacy policy
   cookieForceUpgrade: false,
   deferInitialization: false,
   disableCookies: false,
diff --git a/src/top-domain.js b/src/top-domain.js
index 02c79d1f..bce73ea8 100644
--- a/src/top-domain.js
+++ b/src/top-domain.js
@@ -2,6 +2,7 @@ import baseCookie from './base-cookie';
 import base64Id from './base64Id';
 import getHost from './get-host';
 
+// Utility that finds top level domain to write to
 const topDomain = (url) => {
   const host = getHost(url);
   const parts = host.split('.');

From e948887a31e1011ce8112d6b0319e426c12f8162 Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Thu, 20 Aug 2020 18:47:27 -0700
Subject: [PATCH 2/7] undo unintentional notes

---
 src/notes.md | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/src/notes.md b/src/notes.md
index c9df11e2..c89d8e39 100644
--- a/src/notes.md
+++ b/src/notes.md
@@ -7,11 +7,6 @@
 - Use Amplitude Instrumentation Explorer to help development
   - "Instrumentation" involves matters related to logging user actions (i.e. events)
 
-# Concerns
-- Keep in mind backwards compatibility
-- Keep SDK build size as small as possible
-- Should support as many browsers as possible
-
 # Architecture
 - `index.js` is the main entrypoint of SDK
 - Stubbed methods are used when client imports via `<script>` snippet
@@ -28,24 +23,3 @@
 
 # Development
 - Run `yarn dev` and open `localhost:9000` for development tools in browser
-
-# Misc
-- Readme.io used for documentation
-
-# Ideas
-- E2E browser tests with sauce labs
-- merge with node?
-
-Question
-- Browser compat?
-- MaxMind
-  - Determines region from IP address
-- Some custoemrs wont update SDK
-- Consideration to bring up durign PE weekly monday discussion
-  - browserslist?
-  - eslint plugin compat?
-  - Automation for updating docs when new SDK version is published
-  - Google Tag Manager: Allows people at runtime to add random script tag
-- No plans to stop snippet
-  - might stop require js
-- user & account lookup, allows seeing stream of events sent from users
\ No newline at end of file

From 8df1e2f55ead6c97bd908cd9b5020151887c51b4 Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Fri, 21 Aug 2020 13:47:23 -0700
Subject: [PATCH 3/7] Move and rename notes.md to CONTRIBUTING.md

---
 src/notes.md => CONTRIBUTING.md | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename src/notes.md => CONTRIBUTING.md (100%)

diff --git a/src/notes.md b/CONTRIBUTING.md
similarity index 100%
rename from src/notes.md
rename to CONTRIBUTING.md

From 99453f7bfa42b33f538c65ba06e00bc8e339cb8d Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Fri, 21 Aug 2020 13:47:38 -0700
Subject: [PATCH 4/7] Update CONTRIBUTING.md

---
 CONTRIBUTING.md | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index c89d8e39..9ea6b2aa 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -1,11 +1,12 @@
+**Hi contributor, here are some tips to help you make develop and contribute to the Amplitude SDK for JavaScript** 
+
 # Intro
-- Three ways for SDK to be loaded
+- There are three ways for SDK to be loaded
   - Standard NPM package
   - Snippet in `<script>` tag
   - RequireJS (may be dropped in future)
-- Build system is Make instead of yarn (legacy, may possibly be updated)
-- Use Amplitude Instrumentation Explorer to help development
-  - "Instrumentation" involves matters related to logging user actions (i.e. events)
+- Workflow is facilitated with both `make` and `yarn` (legacy, may possibly be updated to full `yarn`)
+- Chek out the [Amplitude Instrumentation Explorer]((https://chrome.google.com/webstore/detail/amplitude-instrumentation/acehfjhnmhbmgkedjmjlobpgdicnhkbp)) to help logging events during development
 
 # Architecture
 - `index.js` is the main entrypoint of SDK
@@ -21,5 +22,11 @@
     - only applicable to snippet import
     - Can possibly do better?
 
-# Development
-- Run `yarn dev` and open `localhost:9000` for development tools in browser
+# Setting Up Development
+- Cloning, installing, and building
+```
+git clone git@github.com:amplitude/Amplitude-JavaScript.git
+cd Amplitude-JavaScript
+make # Runs tests and generate builds
+yarn dev # Start development utility. Open localhost:9000 in your browser to access
+```

From 729092014d50a0ada614e4f354cda4945229ee59 Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Fri, 21 Aug 2020 13:58:44 -0700
Subject: [PATCH 5/7] Update CONTRIBUTING.md

---
 CONTRIBUTING.md | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 9ea6b2aa..b6382d21 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -1,6 +1,8 @@
-**Hi contributor, here are some tips to help you make develop and contribute to the Amplitude SDK for JavaScript** 
+# Contributing to the Amplitude SDK for JavaScript
+🎉 Thanks for your interest in contributing! 🎉
 
-# Intro
+## Ramping Up
+### Intro
 - There are three ways for SDK to be loaded
   - Standard NPM package
   - Snippet in `<script>` tag
@@ -8,7 +10,7 @@
 - Workflow is facilitated with both `make` and `yarn` (legacy, may possibly be updated to full `yarn`)
 - Chek out the [Amplitude Instrumentation Explorer]((https://chrome.google.com/webstore/detail/amplitude-instrumentation/acehfjhnmhbmgkedjmjlobpgdicnhkbp)) to help logging events during development
 
-# Architecture
+### Architecture
 - `index.js` is the main entrypoint of SDK
 - Stubbed methods are used when client imports via `<script>` snippet
   - Allows app to not be blocked while real JS SDK is loaded in
@@ -20,9 +22,8 @@
 - sameSiteCookie: Sets how public the cookie reading is
   - `amplitude.getInstance() is necessary even during reuse because of snippet stubbed
     - only applicable to snippet import
-    - Can possibly do better?
 
-# Setting Up Development
+### Setting Up Development
 - Cloning, installing, and building
 ```
 git clone git@github.com:amplitude/Amplitude-JavaScript.git

From 6326bbde397d4a818ba2adce669b9345e80771c1 Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Fri, 21 Aug 2020 13:59:12 -0700
Subject: [PATCH 6/7] Update README.md

---
 README.md | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index c9d08ca3..8dc52cf4 100644
--- a/README.md
+++ b/README.md
@@ -13,7 +13,8 @@
 A JavaScript SDK for tracking events and revenue to [Amplitude](https://www.amplitude.com).
 
 ## Installation and Quick Start
-Please visit our :100:[Developer Center](https://developers.amplitude.com/docs/javascript) for instructions on installing and using our the SDK.
+* For using the SDK, please visit our :100:[Developer Center](https://developers.amplitude.com/docs/javascript) for instructions on installing and using our the SDK.
+* For developing the SDK, please visit our [CONTRIBUTING.md](https://github.com/amplitude/Amplitude-JavaScript/blob/master/CONTRIBUTING.md) to get started.
 
 ## Demo Pages
 * A [demo page](https://github.com/amplitude/Amplitude-JavaScript/blob/master/test/browser/amplitudejs.html) showing a simple integration on a web page.
@@ -29,6 +30,9 @@ This library now supports react-native. It has two dependencies on react-native
 * [react-native-device-info](https://www.npmjs.com/package/react-native-device-info) Tested with version 3.1.4
 * [@react-native-community/async-storage](https://www.npmjs.com/package/@react-native-community/async-storage) Tested with version 1.6.2
 
+## Node.js
+Please visit [Amplitude-Node](https://github.com/amplitude/Amplitude-Node) for our Node SDK.
+
 ## Upgrading Major Versions and Breaking Changes #
 
 ### 6.0

From 64d62e2657371f175e610e65a26d5463aa40d82c Mon Sep 17 00:00:00 2001
From: John Tran <jptran318@gmail.com>
Date: Fri, 21 Aug 2020 14:00:05 -0700
Subject: [PATCH 7/7] Reorder README.md

---
 README.md | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index 8dc52cf4..1b395760 100644
--- a/README.md
+++ b/README.md
@@ -21,8 +21,6 @@ A JavaScript SDK for tracking events and revenue to [Amplitude](https://www.ampl
 * A [demo page](https://github.com/amplitude/Amplitude-JavaScript/blob/master/test/browser/amplitudejs-requirejs.html) showing an integration using RequireJS.
 * A [demo page](https://github.com/amplitude/GTM-Web-Demo) demonstrating a potential integration with Google Tag Manager.
 
-## Changelog
-Click [here](https://github.com/amplitude/Amplitude-JavaScript/blob/master/CHANGELOG.md) to view the JavaScript SDK Changelog.
 
 ## React Native
 This library now supports react-native. It has two dependencies on react-native modules you will have to install yourself:
@@ -33,6 +31,9 @@ This library now supports react-native. It has two dependencies on react-native
 ## Node.js
 Please visit [Amplitude-Node](https://github.com/amplitude/Amplitude-Node) for our Node SDK.
 
+## Changelog
+Click [here](https://github.com/amplitude/Amplitude-JavaScript/blob/master/CHANGELOG.md) to view the JavaScript SDK Changelog.
+
 ## Upgrading Major Versions and Breaking Changes #
 
 ### 6.0