Allow use of integrations directly from NPM by zikaari · Pull Request #669 · segmentio/analytics-next

zikaari · 2022-11-10T18:38:07Z

This patch allows customers to import classic integrations directly from NPM, and use those instead of them being loaded from CDN.

This can be useful in a case where one can't afford to have too many concurrent network requests (due to limits imposed by browsers) , and would rather prefer few larger requests instead.

API

import { AnalyticsBrowser } from '@segment/analytics-next'
import GoogleAnalyticsIntegration from '@segment/analytics.js-integration-google-analytics'

// the following rig assumes configuration for Google Analytics will be available in the fetched settings
const analytics = AnalyticsBrowser.load({
	writeKey: '<WRITE_KEY>',
	classicIntegrations: [ GoogleAnalyticsIntegration ]
}),

// if remote settings don't include the integration configuration, it must be supplied directly like so:
const analytics = AnalyticsBrowser.load({
	writeKey: '<WRITE_KEY>',
	classicIntegrations: [ GoogleAnalyticsIntegration ]
}, {
	integrations: {
		'Google Analytics': {
			// all the necessary must be set here
		}
	}
}),

changeset-bot · 2022-11-10T18:38:10Z

🦋 Changeset detected

Latest commit: e00e4ee

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

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Minor

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

zikaari · 2022-11-10T18:38:38Z

I would like to hear opinions on plugins being (Plugin | LegacyIntegrationSource)[], should they be separated? For technical and/or semantic reasons?

silesky · 2022-11-15T01:06:00Z

Honestly, this PR looks really freaking good!

I think we've been moving away from the word "Integration" since it's kind of watery and unspecific (e.g., our addIntegrationMiddleware fn is a no-op). Am I getting this right? cc @pooyaj @chrisradek

silesky · 2022-11-20T22:24:03Z

  // needs to be flushed before plugins are registered
  flushPreBuffer(analytics, preInitBuffer)

+  const legacyIntegrationSources = plugins.filter(


The need for this type guard worries me as it means that any function is automatically determined to be a legacy integration. that raises some public API concerns:

If/when we support locally-imported action destinations (.i.e. non-legacy integrations), going by this pattern we would also want to pass them to plugins. However, if everything is a function, we don't have a clean way to tell them if we treat action destinations similarly, right?

we don't be able to support the future option of allowing generic a plug-in builder function like (analytics: Analytics) => Plugin to be passed to the plugins array.

This suggests that perhaps to do locally imported destinations, we might take one of the approaches:

we create a new key like legacyIntegrationSources: LegacyIntegrationSource[]. It seems like legacyDestinations are a special sort of plugin that gets constructed internally, so maybe it makes sense that they don't belong to "Plugins".

we move logic to an imported adapter that transforms a legacyDestinationSource into a LegacyDestination (plugin subclass) (this might be my favorite from a cleanliness perspective).

wrapped in some way (probably ugly)

Maybe we can mull this over? There's probably an elegant way to resolve this. Open to ideas!

silesky · 2022-11-20T22:57:21Z

@zikaari I did some review that brought me to some reservations about the API, #669 (comment) -- we can continue the discussion in that comment thread.

silesky · 2022-11-21T03:43:23Z

  }
 }

+export function buildIntegration(


This is a pretty nice refactor to break this up! I really like your instinct for abstraction.

chrisradek · 2022-11-28T19:52:05Z

+  // checking for iterable is a quick fix we need in place to prevent
+  // errors showing Iterable as a failed destiantion. Ideally, we should
+  // fix the Iterable metadata instead, but that's a longer process.
+  return (deviceMode || name === 'Segment.io') && name !== 'Iterable'


Segment.io is its own plugin, so shouldn't be included as one of the ajs-destination plugins.

The previous code had the follow check that filters out the Segment.io plugin:

if (name.startsWith('Segment')) { return }

Suspect we can change this to return (deviceMode && name !== 'Iterable') and add a check to exclude Segment at the top of the function.
Edit: Might not even need the check - pretty sure Segment.io won't show as a deviceMode per the checks.

I suggest we do this in a separate pull request. Probably not a safe bet to add too many behavourial changes at once, especially when a behaviour has been there for a long time.

What you have is a behavioral change though. The code is now saying Segment.io is an installable integration. Previously, no Segment plugin would ever make it to this line because we did an early startsWith('Segment') check.

I see, just checked. Since there was an early return does this mean that this has been redundant all along?

Yeah, the Segment.io check was always redundant.

chrisradek

PR is looking great!

chrisradek · 2022-11-28T19:56:37Z

+    globalIntegrations[integrationName] === undefined
+
+  return (
+    integrationName.startsWith('Segment') ||


I think this check can probably be removed if isInstallableIntegrations is updated to exclude Segment.io

#669 (comment)

To me, it makes more sense to have the Segment check in isInstallableIntegrations since Segment is not a classic integration. This function seems to imply you're simply disabling integrations.

True the end result is the same but makes isInstallableIntegrations more truthy.

arielsilvestri · 2022-11-29T16:27:37Z

+    ...Object.entries(remoteIntegrationsConfig)
+      .filter(([name, integrationSettings]) =>
+        isInstallableIntegration(name, integrationSettings)
+      )
+      .map(([name]) => name),


Minor optimization here: might be able to write this as a reduce to only have one scan occur:

...Object.entries(remoteIntegrationsConfig).reduce((accum, [name, integrationSettings]) => isInstallableIntegration(name, integrationSettings) ? [...accum, name] : accum ,[])

Quite clever actually, will do!

Actually, this is even cleaner, and matches the block right after this one:

...Object.keys(remoteIntegrationsConfig).filter((name) => isInstallableIntegration(name, remoteIntegrationsConfig[name]) ),

arielsilvestri · 2022-11-29T16:28:21Z

+    .filter((name) => !shouldSkipIntegration(name, globalIntegrations))
+    .map((name) => {
+      const integrationSettings = remoteIntegrationsConfig[name]
      const version = resolveVersion(integrationSettings)
      const destination = new LegacyDestination(
        name,
        version,
        integrationOptions[name],
-        options
+        options,
+        adhocIntegrationSources?.[name]


Possible same opportunity as above to use reduce

I'd skip it here, just for a "linear" top-down readability sake

chrisradek

Sweet, looks good to me!

zikaari requested review from chrisradek, danieljackins, pooyaj and silesky November 10, 2022 18:38

silesky reviewed Nov 20, 2022

View reviewed changes

silesky reviewed Nov 21, 2022

View reviewed changes

zikaari force-pushed the integrations_from_npm branch from 969854b to da3742c Compare November 22, 2022 23:10

zikaari marked this pull request as ready for review November 22, 2022 23:11

zikaari requested a review from silesky November 22, 2022 23:11

silesky force-pushed the master branch 2 times, most recently from da031b5 to 275d5a3 Compare November 27, 2022 23:54

chrisradek reviewed Nov 28, 2022

View reviewed changes

arielsilvestri reviewed Nov 29, 2022

View reviewed changes

zikaari added 5 commits November 29, 2022 15:49

Allow use of integrations directly from NPM

3e83732

refactor and add tests

3ba93d3

Fix yarn and add changeset

19b8f1f

fix lint error

b74d9d5

Minor refactors and size limit bump

9dc2671

zikaari force-pushed the integrations_from_npm branch from 316f281 to 9dc2671 Compare November 29, 2022 23:52

zikaari requested a review from chrisradek November 30, 2022 00:00

Simplify Segment.io exclusion

e00e4ee

zikaari force-pushed the integrations_from_npm branch from 48003d4 to e00e4ee Compare November 30, 2022 00:03

chrisradek approved these changes Nov 30, 2022

View reviewed changes

silesky approved these changes Nov 30, 2022

View reviewed changes

zikaari merged commit b6ae65b into master Nov 30, 2022

zikaari deleted the integrations_from_npm branch November 30, 2022 18:50

github-actions bot mentioned this pull request Nov 30, 2022

Version Packages #701

Merged

Conversation

zikaari commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

zikaari commented Nov 10, 2022

Uh oh!

silesky commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silesky Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silesky Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zikaari commented Nov 10, 2022 •

edited

Loading

changeset-bot bot commented Nov 10, 2022 •

edited

Loading

silesky commented Nov 15, 2022 •

edited

Loading

silesky Nov 20, 2022 •

edited

Loading

silesky commented Nov 20, 2022 •

edited

Loading

silesky Nov 21, 2022 •

edited

Loading

chrisradek Nov 28, 2022 •

edited

Loading