Refactor: Convert source files to TypeScript #29

HipsterBrown · 2020-07-23T21:30:48Z

/domain @Betterment/js-platform @Betterment/test_track_core
/no-platform

To support how we write front end code at Betterment, this PR converts the source JS files into TypeScript and emits type definitions to ship with the package.

nanda-prbot · 2020-07-23T21:30:54Z

Needs somebody from @Betterment/js-platform and @Betterment/test_track_core to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

_{HOW TO: Claim a Review}

HipsterBrown · 2020-07-23T21:32:48Z

package.json

@@ -6,7 +6,7 @@
  "main": "dist/testTrack.bundle.js",
  "module": "dist/testTrack.js",
  "scripts": {
-    "build": "rollup -c",
+    "build": "tsc && rollup -c",


rollup won't fail the build if TypeScript fails to compile, so running the compiler before will gives us warnings in CI about broken types.

HipsterBrown · 2020-07-23T21:36:02Z

src/abConfiguration.ts

@@ -42,7 +55,7 @@ class ABConfiguration {

    if (splitVariants) {
      const trueVariant = this._getTrueVariant();
-      const trueVariantIndex = splitVariants.indexOf(trueVariant);
+      const trueVariantIndex = splitVariants.indexOf(trueVariant.toString());


splitVariants is an array of strings, so we need to ensure trueVariant is a string.

aburgel · 2020-07-24T16:10:41Z

<< domain

nanda-prbot · 2020-07-24T16:10:45Z

Needs @aburgel to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

_{HOW TO: Give Feedback}

package.json

aburgel · 2020-07-24T16:09:08Z

src/abConfiguration.ts

+  private _visitor: Visitor;
+  private _splitRegistry: SplitRegistry;
+
+  constructor(options: ABConfigurationOptions) {
    if (!options.splitName) {
      throw new Error('must provide splitName');


pardon the typescript ignorance... but does typechecking allow us to remove runtime checks like this? i guess i'm asking whether the ts compiler will add runtime checks for when you are calling this from regular old javascript.

i'm asking whether the ts compiler will add runtime checks for when you are calling this from regular old javascript

The compiler won't do that for us. There are some libraries like io-ts that allow for us to model types and validate them at runtime.

aburgel · 2020-07-24T16:14:42Z

src/assignment.ts

+export type AssignmentOptions = {
+  splitName: string;
+  variant: string | null;
+  context?: string;


i think context is required.

We use this class in TestTrackConfig and context is not passed in there.

src/identifier.ts

src/mixpanelAnalytics.ts

src/varyDSL.ts

aburgel · 2020-07-24T16:45:04Z

src/session.ts


-let loaded = null;
+let loaded: null | ((value?: Visitor | PromiseLike<Visitor>) => void) = null;


for my understanding, is this to support how promises work? you can either pass in the actual resolved object or another promise?

Yes, the type signature for resolve in the Promise constructor is:

declare type PromiseConstructorLike = new <T>(executor: (resolve: (value?: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) => PromiseLike<T>;

Where T stands for our Visitor interface.

src/identifier.ts

aburgel · 2020-07-24T16:50:30Z

src/splitRegistry.ts

    for (let splitName in this.getSplits()) {
-      const split = this._splits[splitName];
+      const split = this.getSplits()[splitName];


👍 good catch, this may have led to bugs with the chrome extension.

src/session.ts

aburgel · 2020-07-24T16:55:27Z

tafn! but i'd love some other typescript eyes on this. @joejansen?

nanda-prbot · 2020-07-24T16:55:32Z

@HipsterBrown needs to incorporate feedback from @aburgel. Bump when done.

_{HOW TO: Resolve Feedback}

HipsterBrown · 2020-07-24T22:37:48Z

Bump

I updated the rollup build to use the official plugins for typescript, node-resolve, and commonjs. The official babel plugin requires using Node v10+, so I didn't introduce that update here.

I also created and exported an AnalyticsProvider interface for the MixpanelAnalytics class to conform and give feedback to users that want to introduce their own analytics client.

nanda-prbot · 2020-07-24T22:37:54Z

Needs @aburgel to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

_{HOW TO: Give Feedback}

aburgel · 2020-07-27T18:27:54Z

rollup.config.js

 import { terser } from 'rollup-plugin-terser';
+import typescript from '@rollup/plugin-typescript';
+
+const customTSConfig = {


looks like this is coming from tsconfig.json. should we just load that file so we don't have to keep them in sync?

Yea, that's a good idea. I want to keep most of the settings from the tsconfig.json but need to override the emitDeclarations setting for the UMD build due to constraints from the the @rollup/plugin-typescript

aburgel · 2020-07-27T18:30:54Z

src/configParser.ts

+
+class ConfigParser {
+  getConfig(): Config {
+    if (typeof window.atob === 'function') {


i think base64 delegates to window.atob if it's available, so we may be able to remove this conditional. (we can do this another time, just pondering out loud)

Yea I'm going to do some refactoring here when I replace base-64 with abab https://app.asana.com/0/786534243298162/1147959806039095

src/mixpanelAnalytics.ts

aburgel · 2020-07-27T20:35:42Z

domainlgtm! nice work!

nanda-prbot · 2020-07-27T20:36:05Z

Approved! 🚢 🤘 🌟

joejansen · 2020-08-03T15:22:48Z

lgtm! great job!

nanda-prbot added the domain-not-claimed label Jul 23, 2020

HipsterBrown commented Jul 23, 2020

View reviewed changes

HipsterBrown force-pushed the nhehr/typescript branch from a6dfc6f to 4b83087 Compare July 23, 2020 21:47

nanda-prbot removed the domain-not-claimed label Jul 24, 2020

aburgel reviewed Jul 24, 2020

View reviewed changes

HipsterBrown added 6 commits July 24, 2020 18:34

refactor: convert source files to typescript

155f07d

chore: add export type declarations with package

0f9bfbc

test: use babel/preset-typescript so jest can run ts files

8ed78f4

chore: use official typescript rollup plugin

944f99b

chore: use official node-resolve, commonjs plugins

b872a5c

refactor: PR feedback

0684ab6

HipsterBrown force-pushed the nhehr/typescript branch from a168eb6 to 0684ab6 Compare July 24, 2020 22:34

aburgel reviewed Jul 27, 2020

View reviewed changes

nanda-prbot added the approved label Jul 27, 2020

refactor: separate analyticsProvider module

2849fc7

HipsterBrown merged commit ddbdff8 into Betterment:master Jul 28, 2020

HipsterBrown deleted the nhehr/typescript branch July 28, 2020 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Convert source files to TypeScript #29

Refactor: Convert source files to TypeScript #29

HipsterBrown commented Jul 23, 2020

nanda-prbot commented Jul 23, 2020

HipsterBrown Jul 23, 2020

HipsterBrown Jul 23, 2020

aburgel commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

aburgel Jul 24, 2020

HipsterBrown Jul 24, 2020

aburgel Jul 24, 2020

HipsterBrown Jul 24, 2020

aburgel Jul 24, 2020

HipsterBrown Jul 24, 2020

aburgel Jul 24, 2020

aburgel commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

HipsterBrown commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

aburgel Jul 27, 2020

HipsterBrown Jul 27, 2020

aburgel Jul 27, 2020

HipsterBrown Jul 27, 2020

aburgel commented Jul 27, 2020

nanda-prbot commented Jul 27, 2020

joejansen commented Aug 3, 2020


		let loaded = null;
		let loaded: null \| ((value?: Visitor \| PromiseLike<Visitor>) => void) = null;

Refactor: Convert source files to TypeScript #29

Refactor: Convert source files to TypeScript #29

Conversation

HipsterBrown commented Jul 23, 2020

nanda-prbot commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aburgel commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aburgel commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

HipsterBrown commented Jul 24, 2020

nanda-prbot commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aburgel commented Jul 27, 2020

nanda-prbot commented Jul 27, 2020

joejansen commented Aug 3, 2020