Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support for Named Exports #41

Closed
fforres opened this issue Jan 14, 2019 · 5 comments · Fixed by #59
Closed

Support for Named Exports #41

fforres opened this issue Jan 14, 2019 · 5 comments · Fixed by #59

Comments

@fforres
Copy link

fforres commented Jan 14, 2019

I came across this while implementing Typewriter on an internal project. After doing some manual refactoring a few times I think it'd be great to have this baked in into Typewriter.

Currently typewriter exports wraps all your tracking calls inside a class you can instance to execute and allows you to pass an analytics instance.
i.e.

Current Status

Show

const genOptions = (context = {}) => ({
  context: {
    ...context,
    typewriter: {
      name: "gen-js",
      version: "5.1.1"
    }
  }
});
export default class Analytics {
  constructor(analytics) {
    this.analytics = analytics || { track: () => null };
  }
  someThingToTrack(props, context) {
    this.analytics.track("Tracking a Thing", props, genOptions(context));
  }
  (... Many other methods)
}

We where using only a few of those tracking methods in our specific project, so we where shipping a lot of unused code that we weren't able to treeshake.
While it might not look like a a huge code reduction if we add the --runtimeValidation false flag, but it is if we want to keep those 😢

So after talking with @colinking on slack, he mentioned making this issue could be a good first step into the discussion.

Proposal

Final export (after running typewriter gen-js with some flags i.e --runtimeValidation false --export class) would look like this:

Show

let analytics = (window && window.analytics) || { track: () => null } 

export const setaAnalytics = (newAnalytics) => { analytics = newAnalytics }

const genOptions = (context = {}) => ({
  context: {
    ...context,
    typewriter: {
      name: 'gen-js',
      version: '5.0.1'
    }
  }
})


export const someThingToTrack = (props, context, callback = () => {}) => {
  analytics.track('Tracking a Thing', props, genOptions(context), callback)
}

Explanation

  • Implements a new flag --export with parameters class & named-exports (or functions?)
    • We set class as the default value for it to keep backwards compatibility
  • Makes every method now a named export
  • Enables a module variable to be the instance of analytics.js
    • Adds method to be able to override that variable
  • Adds support for the callback parameter

Final Thoughts

  • Easier tree shake / dead code elimination
  • Yei for smaller bundle sizes ❤️
@colinking
Copy link
Contributor

Thanks for raising an issue on this, @fforres!

A few questions:

  1. From:

While it might not look like a a huge code reduction if we add the --runtimeValidation false flag, but it is if we want to keep those 😢

Are you all looking to keep the runtimeValidation code in, during production?

  1. How much (in % Kb) would this remove from your bundle?

  2. Just to clarify in this issue (I think we've discussed this privately), there are probably two scenarios of extraneous code here:

  • a) Extraneous calls that are in your JSON Schema / Tracking Plan, but are not currently implemented
  • b) Extraneous calls that are used on /page/a but not on /page/b, and you want to be able to tree-shake these calls from the bundle for /page/b

If the goal of this solution is a), then I would recommend using an event whitelist, since a native solution for whitelisting is on the roadmap (via a versioned lockfile).

But IIUC, you're only worried about b), right? In that case, I'd defer to questions 1) + 2) :)

@fforres
Copy link
Author

fforres commented Jan 14, 2019

Are you all looking to keep the runtimeValidation code in, during production?

Not in our case, no. Now that I think about it, maybe is not something that people would do in their prod bundle.
On the same point, and I might be derailing the conversation. Have we considered doing something like what react does for code elimination?
When bundling their library they look at the env variables and removes all the warnings/dev-only code if NODE_ENV=production, we can keep the warnings on dev and remove them on prod bundling

  1. I'd have to test that. I'll bring some data in a bit :p
    Also, unsure on the size across different plans / users. Maybe our specific tracking plan is small enough that is not important?
    But since our tracking plan is not tied to only one project, It'd be great to allow for the bundling pipeline to make that call for you.

  2. b

I think we can bring more data to make a proper decision, while for a some of our tracking plans it does feel small enough to be negligible, I'm unsure of the impact on some other users / different tracking plans here 🤔

colinking added a commit that referenced this issue Oct 21, 2019
This PR is a complete rebuild of typewriter (called `2.0` or v7) that focuses on improving:

1. Developer experience (DX)
2. Reliability
3. Ease of contribution

## Developer Experience

Typewriter is now built around the idea of a configuration file (`typewriter.yml`) that is used by the CLI to understand how to build your clients. This means that users no longer need to set 4-5 different flags (Tracking Plan ID, directory, workspace slug, etc.) each time they invoke the CLI (without which, the CLI tended to surface difficult-to-understand errors).

To get started, all a user has to know is a single command:

```sh
npx typewriter init
```

This will launch a wizard that walks a user through creating a `typewriter.yml` and then builds their first client.

From then on, a user can just run `npx typewriter` to pull down their latest Tracking Plan and rebuild their clients.

Through this config file, we are able to provide saner defaults, which in Segment's case, allowed us to remove ~30-50 lines of `Makefile` from every typewriter-enabled project.

The entire typewriter CLI was also rebuilt in [Ink](https://github.com/vadimdemedes/ink) so we can  provide a user-friendly CLI with a special focus on easy-to-remedy error handling (f.e., no stack traces).

We've also launched documentation on the Segment Docs site for typewriter: https://segment.com/docs/protocols/typewriter

## Reliability

Previously, when we pushed out new typewriter builds, we would use a combination of snapshot testing on the generated clients and manual testing via a series of example apps in the typewriter repo. We could run the app, which would send off an event or two to Segment, and then we could verify that event showed up correctly in the Segment Debugger.

However, this was problematic because a) it wasn't automated, and therefore didn't always happen and b) it only tested 1-2 example events, so it wasn't exhaustive by any means.

Instead, we now ship a full suite of end-to-end tests that use [`segmentio/mock`](https://github.com/segmentio/mock) to capture analytics events fired by an example app, and then a standardized test suite (`suite.test.ts`) that inspects all of the captured events to verify they match a set of [Joi](https://github.com/hapijs/joi) schemas.

This suite, besides verifying that typewriter generates clients that successfully build, also tests for scenarios that have previously caused issues with new releases of typewriter such as:
- What happens if two events have event names that collide?
- What happens if an event has no explicit properties?
- Is `null` sent to Segment for nullable properties?
- ... etc.

A large benefit of this setup is that the suite itself is language-agnostic, so a new end-to-end test suite can just be an arbitrary app with the Tracking API address updated to `localhost:8765` (`segmentio/mock`). It otherwise requires no testing logic within the example app itself. 🎉 

We've built out an e2e test suite for all supported languages, including `analytics.js` (in JS and TS), `analytics-node` (also in JS + TS) and `analytics-ios` (in Objective-C and Swift).

## Ease of contribution

Adding support for a new language or SDK is now much more straightforward. We've overhauled how client generation works by moving off of [QuickType](https://quicktype.io) (a powerful library for generating JSON serialization/deserialization clients). QuickType was a great starting point for us, but we had to extend their generators in fairly non-obvious ways to customize what code was generated (subclassing fields/logic that was split across the typewriter and QuickType repos, f.e.). This made our generators long, such as the iOS generator which was previously [600 lines](https://github.com/segmentio/typewriter/blob/ec051acdc0c69d814f5636fc5556717e1893c29b/src/commands/gen-ios.ts) (now <300!), and hard to follow -- especially for new contributors.

We decided to take a different approach to generation in favor of readability and reducing the amount of work+context each generator required. With this, we returned to generators based on Handlebars templates. Building a new generator simply involves implementing the various methods of a `Generator` to produce a set of objects that will be passed into your templates.

As part of removing QuickType, we had to roll our own implementations of a few components, specifically:
- **AST**: Previously, we interacted with QuickType's AST which is generalized to support multiple types of schemas (GraphQL, arbitrary JSON, etc.). In our case, we only need to support JSON Schema, so we built our own JSON Schema parser that is opinionated about what fields are relevant to code generation. This produces an easier-to-use type-safe AST, which allows us to separate certain kinds of logic (such as handling unions) into the AST parser rather than in the generators themselves.
- **Namer**: QuickType did a fairly good job of handling name collisions in events and properties, though we did hit some quirks, such as not handling case-insensitive name collisions for files. We now use our own namer for handling name collisions.

With all of this functionality together, adding a new language looks like this:
1. Build the generator by implementing the various `Generator` methods
2. Add an example app to `tests/e2e` that fires off the standard suite of events (see `suite.test.ts`)
3. Document it! Add it to the `README` + the `init` command + Segment docs

## Fixes / New Features

We've also resolved a handful of other issues we've encountered with typewriter, such as:
- Better sane defaults, such as using shared analytics clients by default (`window.analytics`, iOS shared analytics instance, etc.). This means some languages require zero configuration.
- A lack of visibility into typewriter usage, due to a lack of analytics.
- Support for JS Proxies, to avoid program crashes caused by missing analytics methods (f.e., if an event is removed from the Tracking Plan and someone mistakenly syncs it). When this happens, an `Unknown Analytics Event Fired` event will fire that allows teams to alert on this.
- Support for tree-shaking in JS/TS. Resolves #41

## Backwards Compatibility

We haven't ported Android support to this version yet, but will be shipping it soon. For now, we recommend using the Android clients generated by `typewriter@6.1.5`.

The underlying generation logic has heavily changed, so expect the clients to be nearly, but not entirely, compatible with each other. It'll depend on what kind of generated functionality you are using.

## Notes

For more context, see: https://paper.dropbox.com/doc/typewriter-Typewriter-2.0-Vision--AnFffUqElJQQOUHtVLGaWnzbAg-FtUl1rmfXyuoVcI8uS5yM
@fforres
Copy link
Author

fforres commented Oct 22, 2019

❤️ @colinking you rock!! :D
Just looked at TW 2.0
Kudos!! :D

@colinking
Copy link
Contributor

❤️ @fforres

HMU if Brex is interested in trying out Typewriter!

@fforres
Copy link
Author

fforres commented Oct 22, 2019

We are.
I'm a big advocate for protocols / define our tracking-plan ( and tw specially 😄 )
It's taking me more than I hoped for to get someone that can help us with that 😅,
but we are getting there 🤘

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants