fix: handle empty events without compile errors in mobile clients [SC…

…H-1641] (#56) This PR fixes the way that empty events are handled by our mobile Typewriter clients. Previously, iOS and Android clients generated compile-time errors because they expected top-level definitions to be available for empty events, but they were not being generated (since they were `ObjectType` not `ClassType` in QuickType). This PR fixes this by removing the property parameter when an event has no properties declared. If you had a JSON Schema like so: ``` { "description": "This is an empty event.", "name": "Empty Event", "rules": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { "context": {}, "properties": { "type": "object" }, "traits": {} }, "required": ["properties"], "type": "object" } } ``` Then methods like these would be generated: ```objectivec // iOS Objective C - (void)emptyEvent { [self emptyEvent:@{}]; } - (void)emptyEvent:(NSDictionary<NSString *, id> *_Nullable)options { [self.analytics track:@"Empty Event" properties:@{} options:addTypewriterContextFields(options)]; } ``` ```java // Android Java /** * @see <a href="https://segment.com/docs/spec/track/">Track Documentation</a> */ public void emptyEvent() { this.analytics.track("Empty Event", new Properties()); } /** * @see <a href="https://segment.com/docs/spec/track/">Track Documentation</a> */ public void emptyEvent(final @nullable Options options) { this.analytics.track("Empty Event", new Properties(), options); } ``` ```js // TS client emptyEvent( props?: {}, options?: SegmentOptions, callback?: AnalyticsJSCallback ): void; ``` ----- This PR also makes a handful of small refactoring changes: - Updates `quicktype` to pull in the `useList` flag for Android - Refactors the `src/lib` folder to split utils by concept, rather than use a bulk utility folder - Fixes a bug with how event names are generated, where quotes were not properly sanitized. This affected all clients.
segmentio · Mar 5, 2019 · 326e8cc · 326e8cc
1 parent fe44928
commit 326e8cc
Show file tree

Hide file tree

Showing 32 changed files with 249 additions and 184 deletions.
diff --git a/.gitignore b/.gitignore
@@ -73,3 +73,11 @@ dist
 
 # We use yarn.lock
 package-lock.json
+
+.vscode
+
+# Whitelist the two Tracking Plans we use for examples, so that we can test
+# our example apps with other Tracking Plans without accidentally committing.
+examples/local-tracking-plans/*
+!examples/local-tracking-plans/tracking-plan.json
+!examples/local-tracking-plans/tracking-plan-slothgram.json
diff --git a/examples/gen-android/java/build.gradle b/examples/gen-android/java/build.gradle
@@ -7,7 +7,7 @@ buildscript {
         jcenter()
     }
     dependencies {
-        classpath 'com.android.tools.build:gradle:3.2.1'
+        classpath 'com.android.tools.build:gradle:3.3.1'
 
 
         // NOTE: Do not place your application dependencies here; they belong

diff --git a/examples/gen-android/java/gradle/wrapper/gradle-wrapper.properties b/examples/gen-android/java/gradle/wrapper/gradle-wrapper.properties
@@ -1,5 +1,6 @@
+#Wed Feb 27 20:27:56 PST 2019
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.1-all.zip
diff --git a/examples/gen-ios/objectivec/Podfile b/examples/gen-ios/objectivec/Podfile
@@ -2,6 +2,7 @@ use_frameworks!
 
 target 'TypewriterExample' do
   pod "Analytics", "3.6.9"
+  platform :ios, "10.1"
 end
 
 
diff --git a/examples/gen-ios/objectivec/Podfile.lock b/examples/gen-ios/objectivec/Podfile.lock
@@ -11,6 +11,6 @@ SPEC REPOS:
 SPEC CHECKSUMS:
   Analytics: 6541ce337e99d9f7a2240a8b3953940a7be5f998
 
-PODFILE CHECKSUM: 6aa44b1554d25ebfb8e604d56128cde398d87d9e
+PODFILE CHECKSUM: 45b187c979427a842a4adb33f6dfed81443fdd6b
 
 COCOAPODS: 1.5.3
diff --git a/package.json b/package.json
@@ -65,8 +65,8 @@
     "lodash": "^4.17.10",
     "node-fetch": "^2.2.0",
     "omit-deep-lodash": "^1.1.2",
-    "quicktype": "15.0.164",
-    "quicktype-core": "6.0.19",
+    "quicktype": "15.0.174",
+    "quicktype-core": "6.0.26",
     "sort-keys": "^2.0.0",
     "typescript": "^3.2.2",
     "yargs": "^12.0.2"

diff --git a/src/commands/gen-android.ts b/src/commands/gen-android.ts
@@ -9,29 +9,24 @@ import {
   Name,
   TargetLanguage,
   Sourcelike,
-  ArrayType,
-  Type
+  ObjectType
 } from 'quicktype-core'
 
 import { modifySource, SerializedRenderResult } from 'quicktype-core/dist/Source'
-import {
-  OptionValues,
-  BooleanOption,
-  StringOption,
-  EnumOption
-} from 'quicktype-core/dist/RendererOptions'
-import { javaNameStyle } from 'quicktype-core/dist/language/Java'
+import { OptionValues, BooleanOption, StringOption } from 'quicktype-core/dist/RendererOptions'
+import { javaNameStyle, javaOptions } from 'quicktype-core/dist/language/Java'
 
 import {
   getTypedTrackHandler,
   TrackedEvent,
   builder as defaultBuilder,
   Params as DefaultParams
-} from '../lib'
+} from '../lib/cli'
 import * as fs from 'fs'
 import * as util from 'util'
 import { get, map, camelCase, upperFirst } from 'lodash'
 import { AcronymStyleOptions } from 'quicktype-core/dist/support/Acronyms'
+import { getRawName } from '../lib/naming'
 
 const writeFile = util.promisify(fs.writeFile)
 
@@ -63,11 +58,9 @@ export type Params = DefaultParams & {
   language: string
 }
 
-declare const analyticsJavaOptions: {
+declare type analyticsJavaOptions = typeof javaOptions & {
   justTypes: BooleanOption
-  packageName: StringOption
   trackingPlan: StringOption
-  acronymStyle: EnumOption<AcronymStyleOptions>
 }
 
 function toKeyName(name: string) {
@@ -88,7 +81,8 @@ class AnalyticsJavaTargetLanguage extends JavaTargetLanguage {
       justTypes: true,
       packageName: this.packageName,
       trackingPlan: this.trackingPlan,
-      acronymStyle: AcronymStyleOptions.Pascal
+      acronymStyle: AcronymStyleOptions.Pascal,
+      useList: true
     })
   }
   protected get defaultIndentation(): string {
@@ -104,7 +98,7 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
   constructor(
     targetLanguage: TargetLanguage,
     renderContext: RenderContext,
-    protected readonly options: OptionValues<typeof analyticsJavaOptions>
+    protected readonly options: OptionValues<analyticsJavaOptions>
   ) {
     super(targetLanguage, renderContext, options)
   }
@@ -132,13 +126,6 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
     })
   }
 
-  protected javaType(reference: boolean, t: Type, withIssues: boolean = false): Sourcelike {
-    if (t instanceof ArrayType) {
-      return ['List<', this.javaType(false, t.items, withIssues), '>']
-    }
-    return super.javaType(reference, t, withIssues)
-  }
-
   protected emitBuilderSetters(c: ClassType, className: Name): void {
     this.forEachClassProperty(c, 'leading-and-interposing', (name, jsonName, p) => {
       this.emitDescriptionBlock([
@@ -272,32 +259,41 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
     this.finishFile()
   }
 
-  protected emitAnalyticsEventWrapper(name: Name, withOptions: boolean): void {
-    this.emitDescriptionBlock([
-      // TODO: Emit a function description, once we support top-level event descriptions in JSON Schema
-      ['@param props {@link ', name, '} to add extra information to this call.'],
+  protected emitAnalyticsEventWrapper(
+    name: Name,
+    hasProperties: boolean,
+    withOptions: boolean
+  ): void {
+    // TODO: Emit a function description, once we support top-level event descriptions in JSON Schema
+    const description: Sourcelike = [
       ['@see <a href="https://segment.com/docs/spec/track/">Track Documentation</a>']
-    ])
+    ]
+    if (hasProperties) {
+      description.unshift([
+        '@param props {@link ',
+        name,
+        '} to add extra information to this call.'
+      ])
+    }
+    this.emitDescriptionBlock(description)
+
     const camelCaseName = modifySource(camelCase, name)
     this.emitBlock(
       [
         'public void ',
         camelCaseName,
-        '(final @Nullable ',
-        name,
-        ' props',
-        withOptions ? ', final @Nullable Options options' : '',
+        '(',
+        ...(hasProperties ? ['final @Nullable ', name, ' props'] : []),
+        hasProperties && withOptions ? ', ' : '',
+        withOptions ? 'final @Nullable Options options' : '',
         ')'
       ],
       () => {
-        const rawEventName = name
-          .proposeUnstyledNames(new Map())
-          .values()
-          .next().value
         this.emitLine([
           'this.analytics.track("',
-          rawEventName,
-          '", props.toProperties()',
+          getRawName(name),
+          '", ',
+          hasProperties ? 'props.toProperties()' : 'new Properties()',
           withOptions ? ', options' : '',
           ');'
         ])
@@ -335,10 +331,13 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
       })
       this.ensureBlankLine()
 
-      this.forEachTopLevel('leading-and-interposing', (_, name) => {
-        this.emitAnalyticsEventWrapper(name, false)
-        this.ensureBlankLine()
-        this.emitAnalyticsEventWrapper(name, true)
+      this.forEachTopLevel('leading-and-interposing', (t, name) => {
+        if (t instanceof ObjectType) {
+          const hasProperties = t.getProperties().size > 0
+          this.emitAnalyticsEventWrapper(name, hasProperties, false)
+          this.ensureBlankLine()
+          this.emitAnalyticsEventWrapper(name, hasProperties, true)
+        }
       })
     })
   }

diff --git a/src/commands/gen-ios.ts b/src/commands/gen-ios.ts
@@ -10,7 +10,8 @@ import {
   ClassType,
   Name,
   Sourcelike,
-  Type
+  Type,
+  ObjectType
 } from 'quicktype-core'
 import { stringEscape } from 'quicktype-core/dist/support/Strings'
 import { OptionValues, StringOption } from 'quicktype-core/dist/RendererOptions'
@@ -21,7 +22,8 @@ import {
   TrackedEvent,
   builder as defaultBuilder,
   Params as DefaultParams
-} from '../lib'
+} from '../lib/cli'
+import { getRawName } from '../lib/naming'
 import { version } from '../../package.json'
 import * as fs from 'fs'
 import * as util from 'util'
@@ -59,9 +61,8 @@ export type Params = DefaultParams & {
   classPrefix: string
 }
 
-declare const analyticsIOSObjectiveCOptions: typeof objcOptions & {
+declare type analyticsIOSObjectiveCOptions = typeof objcOptions & {
   trackingPlan: StringOption
-  classPrefix: StringOption
 }
 
 class AnalyticsObjectiveCTargetLanguage extends ObjectiveCTargetLanguage {
@@ -97,7 +98,7 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
   constructor(
     targetLanguage: TargetLanguage,
     renderContext: RenderContext,
-    protected readonly options: OptionValues<typeof analyticsIOSObjectiveCOptions>
+    protected readonly options: OptionValues<analyticsIOSObjectiveCOptions>
   ) {
     super(targetLanguage, renderContext, options)
   }
@@ -156,16 +157,23 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
       this.emitLine('- (instancetype)initWithAnalytics:(SEGAnalytics *)analytics;')
       this.ensureBlankLine()
 
-      this.forEachTopLevel('leading-and-interposing', (c: Type, name: Name) => {
-        this.emitDescription(this.descriptionForType(c))
-        this.emitLine(['- (void)', this.variableNameForTopLevel(name), ':(', name, ' *)props;'])
-        this.emitLine([
-          '- (void)',
-          this.variableNameForTopLevel(name),
-          ':(',
-          name,
-          ' *)props withOptions:(NSDictionary<NSString *, id> *_Nullable)options;'
-        ])
+      this.forEachTopLevel('leading-and-interposing', (t: Type, name: Name) => {
+        if (t instanceof ObjectType) {
+          this.emitDescription(this.descriptionForType(t))
+          const hasProperties = t.getProperties().size > 0
+          this.emitLine([
+            '- (void)',
+            this.variableNameForTopLevel(name),
+            ...(hasProperties ? [':(', name, ' *)props;'] : [';'])
+          ])
+          this.emitLine([
+            '- (void)',
+            this.variableNameForTopLevel(name),
+            ':',
+            ...(hasProperties ? ['(', name, ' *)props withOptions:'] : []),
+            '(NSDictionary<NSString *, id> *_Nullable)options;'
+          ])
+        }
       })
     })
   }
@@ -381,40 +389,45 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
     )
   }
 
-  protected emitAnalyticsWrapperMethod(name: Name, options: { withOptions: boolean }) {
+  protected emitAnalyticsWrapperMethod(name: Name, hasProperties: boolean, withOptions: boolean) {
     const camelCaseName = this.variableNameForTopLevel(name)
     this.emitMethod(
       [
         '- (void)',
         camelCaseName,
-        ':(',
-        name,
-        ' *)props',
-        options.withOptions ? ' withOptions:(NSDictionary<NSString *, id> *_Nullable)options' : ''
+        hasProperties || withOptions ? ':' : '',
+        ...(hasProperties ? ['(', name, ' *)props'] : []),
+        hasProperties && withOptions ? ' ' : '',
+        ...(withOptions
+          ? [
+              hasProperties ? 'withOptions:' : '',
+              '(NSDictionary<NSString *, id> *_Nullable)options'
+            ]
+          : [])
       ],
       () => {
-        if (options.withOptions) {
+        if (withOptions) {
           this.emitLine([
             '[self.analytics track:@"',
-            this.rawName(name),
-            '" properties:[props JSONDictionary]',
-            options.withOptions ? ' options:addTypewriterContextFields(options)' : '',
+            getRawName(name),
+            '" properties:',
+            hasProperties ? '[props JSONDictionary]' : '@{}',
+            withOptions ? ' options:addTypewriterContextFields(options)' : '',
             '];'
           ])
         } else {
-          this.emitLine(['[self ', camelCaseName, ':props withOptions:@{}];'])
+          this.emitLine([
+            '[self ',
+            camelCaseName,
+            ':',
+            hasProperties ? 'props withOptions:' : '',
+            '@{}];'
+          ])
         }
       }
     )
   }
 
-  protected rawName(name: Name) {
-    return name
-      .proposeUnstyledNames(new Map())
-      .values()
-      .next().value
-  }
-
   protected emitAnalyticsWrapperImplementation(fileName: string) {
     this.emitImplementation(fileName, () => {
       this.emitMethod('- (instancetype)initWithAnalytics:(SEGAnalytics *)analytics', () => {
@@ -426,9 +439,12 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
       })
       this.ensureBlankLine()
 
-      this.forEachTopLevel('leading-and-interposing', (_: Type, className: Name) => {
-        this.emitAnalyticsWrapperMethod(className, { withOptions: false })
-        this.emitAnalyticsWrapperMethod(className, { withOptions: true })
+      this.forEachTopLevel('leading-and-interposing', (t: Type, className: Name) => {
+        if (t instanceof ObjectType) {
+          const hasProperties = t.getProperties().size > 0
+          this.emitAnalyticsWrapperMethod(className, hasProperties, false)
+          this.emitAnalyticsWrapperMethod(className, hasProperties, true)
+        }
       })
     })
   }

diff --git a/src/commands/gen-js/index.ts b/src/commands/gen-js/index.ts
@@ -1,6 +1,6 @@
-import { getTypedTrackHandler } from '../../lib'
+import { getTypedTrackHandler } from '../../lib/cli'
 import { ModuleKind, ScriptTarget } from 'typescript'
-import { builder as defaultBuilder, Params as DefaultParams } from '../../lib'
+import { builder as defaultBuilder, Params as DefaultParams } from '../../lib/cli'
 import * as util from 'util'
 import * as fs from 'fs'
 const writeFile = util.promisify(fs.writeFile)