Create rc of analytics node #587

silesky · 2022-08-24T19:33:37Z

Sorry for this huge PR

Changelog:

copy / refactor logic from core and use inside of nodejs
adds plugin-validation package
adds internal config package (for jest stuff and lint-staged)
adds core-integration-test package
adds better test DX experience

Fast follow:

Add node tests / example that use express
refactor other repos to use internal config and incremental building
Update browser to use the analytics-core interfaces
- Refactor analytics to use / extend Core classes and interfaces -- register segment inspector via event emitter.
- Node tests that rely on stats can be refactored to use event emitter instead.
Refactor CI to acknowledge additional packages (will use turbo this time).

changeset-bot · 2022-08-24T19:33:40Z

🦋 Changeset detected

Latest commit: 8b2cf19

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@segment/analytics-plugin-validation	Major
@segment/analytics-core	Minor
@segment/analytics-node	Patch
@segment/analytics-next	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

silesky · 2022-08-24T22:13:37Z

packages/core/src/priority-queue/persisted.ts

+  )
+}
+
+function remove(loc: Storage, key: string): void {


Not necessary to be in core (the tests weren’t as brittle as I thought), but I refactored this entire file to no longer create any import side effects (takes loc as an argument -- the cache is only created when PersistedPriorityQueue is instantiated).

chrisradek · 2022-08-28T23:36:07Z

Started going through this PR, wanted to know about this point:

copy / refactor logic from core and use inside of nodejs

Can you share what was refactored vs what was just copied? That'll just help with knowing what to focus extra attention on.

silesky · 2022-08-29T01:48:24Z

packages/core/src/priority-queue/persisted.ts

@@ -0,0 +1,136 @@
+import { PriorityQueue } from '.'


File refactored to have no import-time side effects -- it's not relevant to node, but I went down that path and thought I would sneak it in as a "core refactor".

silesky · 2022-08-29T01:50:04Z

packages/core/src/plugins/index.ts

+  | 'destination'
+  | 'enrichment'
+  | 'utility'
+


This file is basically basically unchanged, other than generics

silesky · 2022-08-29T01:51:05Z

packages/core/src/events/interfaces.ts

+  All?: boolean
+  [integration: string]: boolean | JSONObject | undefined
+}
+


Other than the word "core", materially unchanged

silesky · 2022-08-29T01:53:16Z

packages/core/src/connection/__tests__/index.test.ts

@@ -0,0 +1,32 @@
+/**
+ * @jest-environment jsdom
+ */


Declaring tests using this magic comment allows us to easily identify where our dom dependencies are

silesky · 2022-08-29T01:54:31Z

packages/core/src/arguments-resolver/page.ts

+import { isFunction, isPlainObject, isString } from '../validation'
+import { JSONObject, EventProperties, CoreOptions } from '../events'
+import { Callback } from '../events/interfaces'
+


Basically unchanged, except for moving the arguments resolver to its own "page" file -- it's the only resolver we need to bring in (because of the overloads)

silesky · 2022-08-29T01:57:05Z

internal/config/src/jest/index.js

@@ -0,0 +1,18 @@
+const getPackages = require('get-monorepo-packages')
+


This module allows ts-jest to pick up changes in other packages without having to run build on every external change.

silesky · 2022-08-29T02:04:01Z

packages/core/src/context/index.ts

+  }
+}
+
+export class CoreContext<Event extends CoreSegmentEvent = CoreSegmentEvent> {


refactored from the browser Context class to make this new "base" class (meant to be used by node and browser) -- adds a generic, improves decoupling by taking Stats and Logger via constructor injection, removes extraneous access modifiers like the word "public" to make a bit prettier.

silesky · 2022-08-29T02:04:29Z

packages/core/src/emitter/index.ts

@@ -1,12 +1,12 @@
-export class Emitter {
-  private callbacks: Record<string, Function[]> = {}
+export class Emitter<EventName extends string = string> {


Change to allow one to specific events

silesky · 2022-08-29T02:05:42Z

packages/core/src/events/index.ts

+      options: {},
+    }
+
+    if (!this.user) return base


the only material change to this file IIRC

silesky · 2022-08-29T02:08:14Z

packages/core/src/analytics/dispatch.ts

+export async function dispatch(
+  event: CoreSegmentEvent,
+  queue: EventQueue,
+  emitter: Emitter,


This argument is new -- will allow us to decouple this function from listeners like segment inspector

Does it make sense for the emitter to ever not be analytics? Wonder if naming the argument something like analytics would make it more clear what should be passed/what's emitting events.

I see your point, but IMO it's cleaner overall and will encourage better unit testing if the name / arg type reflects only the minimal required interface, per "interface segregation principal". I think it's best to be clear about the actual required semantics of the things that's being passed -- it always ends up makes refactoring easier later on so you don't end up accidentally relying on behavior that you don't use.

To be clear - I was expecting the type to still be Emitter, just the name of the property to be analytics. Can we have a doc string then so someone looking at the function understands we expect it to be analytics (even if unit tests will pass anything)

Sure, we can have a doc string.

I think ISP can still be a useful concept when it comes to variable naming (see vanilla JS / dynamic languages), as it's about not exposing unnecessary "stuff" to a consumer, which can include ideas as well as actual type information -- if I was to call dispatch with new Emitter() but the arg name part of type signature said analytics, I think that would be a bit confusing for a new dev, even if it just for a test -- they might think it's a mistake, and seek to refactor it to take in an Analytics interface.

I want to be extra explicit (in both the variable name and type) that we don't need anything like an Analytics instance and all the baggage it implies, that they are fully decoupled. In the future, we may limit it further to only accept an interface with the emit function.

silesky · 2022-08-29T02:09:26Z

Started going through this PR, wanted to know about this point:

copy / refactor logic from core and use inside of nodejs

Can you share what was refactored vs what was just copied? That'll just help with knowing what to focus extra attention on.

Good call out. I added PR comments to the relevant files to review!

silesky · 2022-08-29T02:15:06Z

packages/core/src/queue/event-queue.ts

+    try {
+      ctx = await this.flushOne(ctx)
+      const done = Date.now() - start
+      this.emit('delivery_success', done)


new event emitted for inspector host to listen to -- I could see us also moving the stats stuff outside of this function altogether and just rely on emitter events.

/cc @zikaari

silesky · 2022-08-29T02:18:02Z

packages/core/src/queue/event-queue.ts

+
+    this.emit('message_delivered', ctx)
+    ctx.stats?.increment('message_delivered')
+    // should emit to: TODO: inspectorHost.delivered?.(ctx as any, ['segment.io']) -- can we move all this inspector stuff to a plugin that listens to these events?


We don't want inspectorHost tightly coupled -- it can use the event emitter to listen to events. I can see it as a plugin.

Sounds like a utility plugin - /cc @zikaari

silesky · 2022-08-29T02:19:00Z

packages/node/src/app/analytics-node.ts

+    this.integrations = options?.integrations ?? {}
+    this.options = options ?? {}
+    this.queue = queue
+    bindAll(this)


silesky · 2022-08-29T02:19:56Z

packages/node/src/app/analytics-node.ts

+    const registrations = plugins.map((xt) =>
+      this.queue.register(ctx, xt, this)
+    )
+    await Promise.all(registrations)


we might want to handle these errors, but left as is. Weirdly, no lint complaints.

silesky · 2022-08-29T02:24:46Z

packages/core-integration-tests/package.json

@@ -0,0 +1,20 @@
+{


new package!

silesky · 2022-08-29T17:26:55Z

package.json

    "postpublish-run-all": "yarn workspaces foreach -vpt --no-private run postpublish",
    "version-run-all": "yarn workspaces foreach -vpt --no-private run version",
    "core": "yarn workspace @segment/analytics-core",
-    "core+deps": "turbo run --filter='analytics-core'",
+    "core+deps": "turbo run --filter=@segment/analytics-core...",


the "..." syntax is how turborepo expresses "include all dependencies".

silesky · 2022-08-29T17:31:50Z

packages/core/jest.config.js

+  setupFilesAfterEnv: ['./jest.setup.js'],
+  projects: [
+    "<rootDir>",
+    "<rootDir>/../core-integration-tests"


running "yarn test" and so forth in core automatically runs integration tests (yes, jest has its own support for monorepos!)

chrisradek

Overall I think this looks good - thanks for commenting on what was changed vs copied.

constraints.pro

package.json

chrisradek · 2022-08-30T03:10:00Z

packages/node/src/__tests__/integration.test.ts

+    it('populates anonymousId if provided', async () => {
+      const [analytics] = await load({ writeKey })
+
+      const ctx = await analytics.alias('chris radek', 'chris', {


My test cases live on!

packages/core/src/callback/index.ts

packages/node/src/app/analytics-node.ts

chrisradek · 2022-08-30T04:43:41Z

packages/core/src/queue/event-queue.ts

+    try {
+      ctx = await this.flushOne(ctx)
+      const done = Date.now() - start
+      this.emit('delivery_success', done)


/cc @zikaari

packages/core/src/queue/event-queue.ts

chrisradek · 2022-08-30T04:45:58Z

packages/core/src/queue/event-queue.ts

+
+    this.emit('message_delivered', ctx)
+    ctx.stats?.increment('message_delivered')
+    // should emit to: TODO: inspectorHost.delivered?.(ctx as any, ['segment.io']) -- can we move all this inspector stuff to a plugin that listens to these events?


Sounds like a utility plugin - /cc @zikaari

packages/core/src/analytics/dispatch.ts

packages/node/src/index.ts

chrisradek

Thanks for going through the feedback and making updates! Looks good to me!

silesky force-pushed the create-rc-of-analytics-node branch from 6293183 to 06bc149 Compare August 24, 2022 19:37

silesky requested review from pooyaj and chrisradek August 24, 2022 19:50

silesky commented Aug 24, 2022

View reviewed changes

silesky force-pushed the create-rc-of-analytics-node branch 4 times, most recently from 027c99f to 21106d6 Compare August 27, 2022 19:15

silesky force-pushed the create-rc-of-analytics-node branch from 2d40546 to 862c993 Compare August 28, 2022 23:39

silesky commented Aug 29, 2022

View reviewed changes

packages/core-integration-tests/package.json

@@ -0,0 +1,20 @@

{

Copy link

Contributor Author

silesky Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new package!

silesky commented Aug 29, 2022

View reviewed changes

silesky force-pushed the create-rc-of-analytics-node branch from c879f56 to 6dcd98c Compare August 29, 2022 18:09

chrisradek reviewed Aug 30, 2022

View reviewed changes

silesky force-pushed the create-rc-of-analytics-node branch from ef8f63b to 4f3ca6f Compare August 30, 2022 16:47

create analytics-node package

7e3f3d6

silesky force-pushed the create-rc-of-analytics-node branch 3 times, most recently from 76f0322 to 133da09 Compare September 2, 2022 22:39

add jest module map

3880fa0

silesky force-pushed the create-rc-of-analytics-node branch from 133da09 to 3880fa0 Compare September 2, 2022 22:51

add module map to other packages

25bd37a

silesky force-pushed the create-rc-of-analytics-node branch 2 times, most recently from ec57ee9 to d34892a Compare September 3, 2022 04:29

use createJestConfig fn

081045b

silesky force-pushed the create-rc-of-analytics-node branch from d34892a to 081045b Compare September 3, 2022 04:32

add lint-staged-shared-config

1bae417

silesky requested review from chrisradek and zikaari September 6, 2022 18:42

silesky force-pushed the create-rc-of-analytics-node branch 2 times, most recently from 0a6bf55 to cd689d2 Compare September 6, 2022 18:51

add internal README

8b2cf19

silesky force-pushed the create-rc-of-analytics-node branch from cd689d2 to 8b2cf19 Compare September 6, 2022 18:52

chrisradek approved these changes Sep 6, 2022

View reviewed changes

silesky merged commit 598fc31 into master Sep 6, 2022

silesky deleted the create-rc-of-analytics-node branch September 6, 2022 20:49

github-actions bot mentioned this pull request Sep 6, 2022

Version Packages #591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create rc of analytics node #587

Create rc of analytics node #587

silesky commented Aug 24, 2022 •

edited

Loading

changeset-bot bot commented Aug 24, 2022 •

edited

Loading

silesky Aug 24, 2022 •

edited

Loading

chrisradek commented Aug 28, 2022

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022

chrisradek Aug 30, 2022

silesky Aug 30, 2022 •

edited

Loading

chrisradek Aug 30, 2022

silesky Aug 30, 2022 •

edited

Loading

silesky commented Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

chrisradek Aug 30, 2022

silesky Aug 29, 2022 •

edited

Loading

chrisradek Aug 30, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022

silesky Aug 29, 2022

silesky Aug 29, 2022

chrisradek left a comment

chrisradek Aug 30, 2022

chrisradek Aug 30, 2022

chrisradek Aug 30, 2022

chrisradek left a comment

		@@ -0,0 +1,18 @@
		const getPackages = require('get-monorepo-packages')

Create rc of analytics node #587

Create rc of analytics node #587

Conversation

silesky commented Aug 24, 2022 • edited Loading

changeset-bot bot commented Aug 24, 2022 • edited Loading

🦋 Changeset detected

silesky Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

chrisradek commented Aug 28, 2022

silesky Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

silesky Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Aug 29, 2022 • edited Loading

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

silesky Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

silesky commented Aug 29, 2022 • edited Loading

silesky Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Aug 29, 2022 • edited Loading

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

chrisradek left a 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

chrisradek left a comment

Choose a reason for hiding this comment

silesky commented Aug 24, 2022 •

edited

Loading

changeset-bot bot commented Aug 24, 2022 •

edited

Loading

silesky Aug 24, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 30, 2022 •

edited

Loading

silesky Aug 30, 2022 •

edited

Loading

silesky commented Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading

silesky Aug 29, 2022 •

edited

Loading