Documentation Updates

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.

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(

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) {

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');

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

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();
 }

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);

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

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,

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('.');