Skip to content

Commit 326e8cc

Browse files
authored
fix: handle empty events without compile errors in mobile clients [SCH-1641] (segmentio#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.
1 parent fe44928 commit 326e8cc

32 files changed

+249
-184
lines changed

.gitignore

+8
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,11 @@ dist
7373

7474
# We use yarn.lock
7575
package-lock.json
76+
77+
.vscode
78+
79+
# Whitelist the two Tracking Plans we use for examples, so that we can test
80+
# our example apps with other Tracking Plans without accidentally committing.
81+
examples/local-tracking-plans/*
82+
!examples/local-tracking-plans/tracking-plan.json
83+
!examples/local-tracking-plans/tracking-plan-slothgram.json

examples/gen-android/java/build.gradle

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ buildscript {
77
jcenter()
88
}
99
dependencies {
10-
classpath 'com.android.tools.build:gradle:3.2.1'
10+
classpath 'com.android.tools.build:gradle:3.3.1'
1111

1212

1313
// NOTE: Do not place your application dependencies here; they belong
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
#Wed Feb 27 20:27:56 PST 2019
12
distributionBase=GRADLE_USER_HOME
23
distributionPath=wrapper/dists
3-
distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip
44
zipStoreBase=GRADLE_USER_HOME
55
zipStorePath=wrapper/dists
6+
distributionUrl=https\://services.gradle.org/distributions/gradle-4.10.1-all.zip

examples/gen-ios/objectivec/Podfile

+1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use_frameworks!
22

33
target 'TypewriterExample' do
44
pod "Analytics", "3.6.9"
5+
platform :ios, "10.1"
56
end
67

78

examples/gen-ios/objectivec/Podfile.lock

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,6 @@ SPEC REPOS:
1111
SPEC CHECKSUMS:
1212
Analytics: 6541ce337e99d9f7a2240a8b3953940a7be5f998
1313

14-
PODFILE CHECKSUM: 6aa44b1554d25ebfb8e604d56128cde398d87d9e
14+
PODFILE CHECKSUM: 45b187c979427a842a4adb33f6dfed81443fdd6b
1515

1616
COCOAPODS: 1.5.3

package.json

+2-2
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,8 @@
6565
"lodash": "^4.17.10",
6666
"node-fetch": "^2.2.0",
6767
"omit-deep-lodash": "^1.1.2",
68-
"quicktype": "15.0.164",
69-
"quicktype-core": "6.0.19",
68+
"quicktype": "15.0.174",
69+
"quicktype-core": "6.0.26",
7070
"sort-keys": "^2.0.0",
7171
"typescript": "^3.2.2",
7272
"yargs": "^12.0.2"

src/commands/gen-android.ts

+40-41
Original file line numberDiff line numberDiff line change
@@ -9,29 +9,24 @@ import {
99
Name,
1010
TargetLanguage,
1111
Sourcelike,
12-
ArrayType,
13-
Type
12+
ObjectType
1413
} from 'quicktype-core'
1514

1615
import { modifySource, SerializedRenderResult } from 'quicktype-core/dist/Source'
17-
import {
18-
OptionValues,
19-
BooleanOption,
20-
StringOption,
21-
EnumOption
22-
} from 'quicktype-core/dist/RendererOptions'
23-
import { javaNameStyle } from 'quicktype-core/dist/language/Java'
16+
import { OptionValues, BooleanOption, StringOption } from 'quicktype-core/dist/RendererOptions'
17+
import { javaNameStyle, javaOptions } from 'quicktype-core/dist/language/Java'
2418

2519
import {
2620
getTypedTrackHandler,
2721
TrackedEvent,
2822
builder as defaultBuilder,
2923
Params as DefaultParams
30-
} from '../lib'
24+
} from '../lib/cli'
3125
import * as fs from 'fs'
3226
import * as util from 'util'
3327
import { get, map, camelCase, upperFirst } from 'lodash'
3428
import { AcronymStyleOptions } from 'quicktype-core/dist/support/Acronyms'
29+
import { getRawName } from '../lib/naming'
3530

3631
const writeFile = util.promisify(fs.writeFile)
3732

@@ -63,11 +58,9 @@ export type Params = DefaultParams & {
6358
language: string
6459
}
6560

66-
declare const analyticsJavaOptions: {
61+
declare type analyticsJavaOptions = typeof javaOptions & {
6762
justTypes: BooleanOption
68-
packageName: StringOption
6963
trackingPlan: StringOption
70-
acronymStyle: EnumOption<AcronymStyleOptions>
7164
}
7265

7366
function toKeyName(name: string) {
@@ -88,7 +81,8 @@ class AnalyticsJavaTargetLanguage extends JavaTargetLanguage {
8881
justTypes: true,
8982
packageName: this.packageName,
9083
trackingPlan: this.trackingPlan,
91-
acronymStyle: AcronymStyleOptions.Pascal
84+
acronymStyle: AcronymStyleOptions.Pascal,
85+
useList: true
9286
})
9387
}
9488
protected get defaultIndentation(): string {
@@ -104,7 +98,7 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
10498
constructor(
10599
targetLanguage: TargetLanguage,
106100
renderContext: RenderContext,
107-
protected readonly options: OptionValues<typeof analyticsJavaOptions>
101+
protected readonly options: OptionValues<analyticsJavaOptions>
108102
) {
109103
super(targetLanguage, renderContext, options)
110104
}
@@ -132,13 +126,6 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
132126
})
133127
}
134128

135-
protected javaType(reference: boolean, t: Type, withIssues: boolean = false): Sourcelike {
136-
if (t instanceof ArrayType) {
137-
return ['List<', this.javaType(false, t.items, withIssues), '>']
138-
}
139-
return super.javaType(reference, t, withIssues)
140-
}
141-
142129
protected emitBuilderSetters(c: ClassType, className: Name): void {
143130
this.forEachClassProperty(c, 'leading-and-interposing', (name, jsonName, p) => {
144131
this.emitDescriptionBlock([
@@ -272,32 +259,41 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
272259
this.finishFile()
273260
}
274261

275-
protected emitAnalyticsEventWrapper(name: Name, withOptions: boolean): void {
276-
this.emitDescriptionBlock([
277-
// TODO: Emit a function description, once we support top-level event descriptions in JSON Schema
278-
['@param props {@link ', name, '} to add extra information to this call.'],
262+
protected emitAnalyticsEventWrapper(
263+
name: Name,
264+
hasProperties: boolean,
265+
withOptions: boolean
266+
): void {
267+
// TODO: Emit a function description, once we support top-level event descriptions in JSON Schema
268+
const description: Sourcelike = [
279269
['@see <a href="https://segment.com/docs/spec/track/">Track Documentation</a>']
280-
])
270+
]
271+
if (hasProperties) {
272+
description.unshift([
273+
'@param props {@link ',
274+
name,
275+
'} to add extra information to this call.'
276+
])
277+
}
278+
this.emitDescriptionBlock(description)
279+
281280
const camelCaseName = modifySource(camelCase, name)
282281
this.emitBlock(
283282
[
284283
'public void ',
285284
camelCaseName,
286-
'(final @Nullable ',
287-
name,
288-
' props',
289-
withOptions ? ', final @Nullable Options options' : '',
285+
'(',
286+
...(hasProperties ? ['final @Nullable ', name, ' props'] : []),
287+
hasProperties && withOptions ? ', ' : '',
288+
withOptions ? 'final @Nullable Options options' : '',
290289
')'
291290
],
292291
() => {
293-
const rawEventName = name
294-
.proposeUnstyledNames(new Map())
295-
.values()
296-
.next().value
297292
this.emitLine([
298293
'this.analytics.track("',
299-
rawEventName,
300-
'", props.toProperties()',
294+
getRawName(name),
295+
'", ',
296+
hasProperties ? 'props.toProperties()' : 'new Properties()',
301297
withOptions ? ', options' : '',
302298
');'
303299
])
@@ -335,10 +331,13 @@ class AnalyticsJavaWrapperRenderer extends JavaRenderer {
335331
})
336332
this.ensureBlankLine()
337333

338-
this.forEachTopLevel('leading-and-interposing', (_, name) => {
339-
this.emitAnalyticsEventWrapper(name, false)
340-
this.ensureBlankLine()
341-
this.emitAnalyticsEventWrapper(name, true)
334+
this.forEachTopLevel('leading-and-interposing', (t, name) => {
335+
if (t instanceof ObjectType) {
336+
const hasProperties = t.getProperties().size > 0
337+
this.emitAnalyticsEventWrapper(name, hasProperties, false)
338+
this.ensureBlankLine()
339+
this.emitAnalyticsEventWrapper(name, hasProperties, true)
340+
}
342341
})
343342
})
344343
}

src/commands/gen-ios.ts

+51-35
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ import {
1010
ClassType,
1111
Name,
1212
Sourcelike,
13-
Type
13+
Type,
14+
ObjectType
1415
} from 'quicktype-core'
1516
import { stringEscape } from 'quicktype-core/dist/support/Strings'
1617
import { OptionValues, StringOption } from 'quicktype-core/dist/RendererOptions'
@@ -21,7 +22,8 @@ import {
2122
TrackedEvent,
2223
builder as defaultBuilder,
2324
Params as DefaultParams
24-
} from '../lib'
25+
} from '../lib/cli'
26+
import { getRawName } from '../lib/naming'
2527
import { version } from '../../package.json'
2628
import * as fs from 'fs'
2729
import * as util from 'util'
@@ -59,9 +61,8 @@ export type Params = DefaultParams & {
5961
classPrefix: string
6062
}
6163

62-
declare const analyticsIOSObjectiveCOptions: typeof objcOptions & {
64+
declare type analyticsIOSObjectiveCOptions = typeof objcOptions & {
6365
trackingPlan: StringOption
64-
classPrefix: StringOption
6566
}
6667

6768
class AnalyticsObjectiveCTargetLanguage extends ObjectiveCTargetLanguage {
@@ -97,7 +98,7 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
9798
constructor(
9899
targetLanguage: TargetLanguage,
99100
renderContext: RenderContext,
100-
protected readonly options: OptionValues<typeof analyticsIOSObjectiveCOptions>
101+
protected readonly options: OptionValues<analyticsIOSObjectiveCOptions>
101102
) {
102103
super(targetLanguage, renderContext, options)
103104
}
@@ -156,16 +157,23 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
156157
this.emitLine('- (instancetype)initWithAnalytics:(SEGAnalytics *)analytics;')
157158
this.ensureBlankLine()
158159

159-
this.forEachTopLevel('leading-and-interposing', (c: Type, name: Name) => {
160-
this.emitDescription(this.descriptionForType(c))
161-
this.emitLine(['- (void)', this.variableNameForTopLevel(name), ':(', name, ' *)props;'])
162-
this.emitLine([
163-
'- (void)',
164-
this.variableNameForTopLevel(name),
165-
':(',
166-
name,
167-
' *)props withOptions:(NSDictionary<NSString *, id> *_Nullable)options;'
168-
])
160+
this.forEachTopLevel('leading-and-interposing', (t: Type, name: Name) => {
161+
if (t instanceof ObjectType) {
162+
this.emitDescription(this.descriptionForType(t))
163+
const hasProperties = t.getProperties().size > 0
164+
this.emitLine([
165+
'- (void)',
166+
this.variableNameForTopLevel(name),
167+
...(hasProperties ? [':(', name, ' *)props;'] : [';'])
168+
])
169+
this.emitLine([
170+
'- (void)',
171+
this.variableNameForTopLevel(name),
172+
':',
173+
...(hasProperties ? ['(', name, ' *)props withOptions:'] : []),
174+
'(NSDictionary<NSString *, id> *_Nullable)options;'
175+
])
176+
}
169177
})
170178
})
171179
}
@@ -381,40 +389,45 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
381389
)
382390
}
383391

384-
protected emitAnalyticsWrapperMethod(name: Name, options: { withOptions: boolean }) {
392+
protected emitAnalyticsWrapperMethod(name: Name, hasProperties: boolean, withOptions: boolean) {
385393
const camelCaseName = this.variableNameForTopLevel(name)
386394
this.emitMethod(
387395
[
388396
'- (void)',
389397
camelCaseName,
390-
':(',
391-
name,
392-
' *)props',
393-
options.withOptions ? ' withOptions:(NSDictionary<NSString *, id> *_Nullable)options' : ''
398+
hasProperties || withOptions ? ':' : '',
399+
...(hasProperties ? ['(', name, ' *)props'] : []),
400+
hasProperties && withOptions ? ' ' : '',
401+
...(withOptions
402+
? [
403+
hasProperties ? 'withOptions:' : '',
404+
'(NSDictionary<NSString *, id> *_Nullable)options'
405+
]
406+
: [])
394407
],
395408
() => {
396-
if (options.withOptions) {
409+
if (withOptions) {
397410
this.emitLine([
398411
'[self.analytics track:@"',
399-
this.rawName(name),
400-
'" properties:[props JSONDictionary]',
401-
options.withOptions ? ' options:addTypewriterContextFields(options)' : '',
412+
getRawName(name),
413+
'" properties:',
414+
hasProperties ? '[props JSONDictionary]' : '@{}',
415+
withOptions ? ' options:addTypewriterContextFields(options)' : '',
402416
'];'
403417
])
404418
} else {
405-
this.emitLine(['[self ', camelCaseName, ':props withOptions:@{}];'])
419+
this.emitLine([
420+
'[self ',
421+
camelCaseName,
422+
':',
423+
hasProperties ? 'props withOptions:' : '',
424+
'@{}];'
425+
])
406426
}
407427
}
408428
)
409429
}
410430

411-
protected rawName(name: Name) {
412-
return name
413-
.proposeUnstyledNames(new Map())
414-
.values()
415-
.next().value
416-
}
417-
418431
protected emitAnalyticsWrapperImplementation(fileName: string) {
419432
this.emitImplementation(fileName, () => {
420433
this.emitMethod('- (instancetype)initWithAnalytics:(SEGAnalytics *)analytics', () => {
@@ -426,9 +439,12 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
426439
})
427440
this.ensureBlankLine()
428441

429-
this.forEachTopLevel('leading-and-interposing', (_: Type, className: Name) => {
430-
this.emitAnalyticsWrapperMethod(className, { withOptions: false })
431-
this.emitAnalyticsWrapperMethod(className, { withOptions: true })
442+
this.forEachTopLevel('leading-and-interposing', (t: Type, className: Name) => {
443+
if (t instanceof ObjectType) {
444+
const hasProperties = t.getProperties().size > 0
445+
this.emitAnalyticsWrapperMethod(className, hasProperties, false)
446+
this.emitAnalyticsWrapperMethod(className, hasProperties, true)
447+
}
432448
})
433449
})
434450
}

src/commands/gen-js/index.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
import { getTypedTrackHandler } from '../../lib'
1+
import { getTypedTrackHandler } from '../../lib/cli'
22
import { ModuleKind, ScriptTarget } from 'typescript'
3-
import { builder as defaultBuilder, Params as DefaultParams } from '../../lib'
3+
import { builder as defaultBuilder, Params as DefaultParams } from '../../lib/cli'
44
import * as util from 'util'
55
import * as fs from 'fs'
66
const writeFile = util.promisify(fs.writeFile)

0 commit comments

Comments
 (0)