-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow use of integrations directly from NPM #669
Conversation
🦋 Changeset detectedLatest commit: e00e4ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
I would like to hear opinions on |
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 |
@@ -279,12 +283,21 @@ async function loadAnalytics( | |||
// needs to be flushed before plugins are registered | |||
flushPreBuffer(analytics, preInitBuffer) | |||
|
|||
const legacyIntegrationSources = plugins.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@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. |
@@ -29,14 +33,37 @@ function recordLoadMetrics(fullPath: string, ctx: Context, name: string): void { | |||
} | |||
} | |||
|
|||
export function buildIntegration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty nice refactor to break this up! I really like your instinct for abstraction.
969854b
to
da3742c
Compare
da031b5
to
275d5a3
Compare
// 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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, just checked. Since there was an early return does this mean that this has been redundant all along?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the Segment.io check was always redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is looking great!
globalIntegrations[integrationName] === undefined | ||
|
||
return ( | ||
integrationName.startsWith('Segment') || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check can probably be removed if isInstallableIntegrations
is updated to exclude Segment.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...Object.entries(remoteIntegrationsConfig) | ||
.filter(([name, integrationSettings]) => | ||
isInstallableIntegration(name, integrationSettings) | ||
) | ||
.map(([name]) => name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
,[])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite clever actually, will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is even cleaner, and matches the block right after this one:
...Object.keys(remoteIntegrationsConfig).filter((name) =>
isInstallableIntegration(name, remoteIntegrationsConfig[name])
),
.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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible same opportunity as above to use reduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd skip it here, just for a "linear" top-down readability sake
316f281
to
9dc2671
Compare
48003d4
to
e00e4ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, looks good to me!
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