-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add ability to use action-destinations from NPM #862
Conversation
🦋 Changeset detectedLatest commit: 612d815 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 |
@@ -278,7 +284,16 @@ async function loadAnalytics( | |||
|
|||
attachInspector(analytics) | |||
|
|||
const plugins = settings.plugins ?? [] | |||
const plugins = settings.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.
Nit: Can we move this plugins filtering stuff from here into registerPlugins()
-
loadAnalytics
has a habit of getting large (it does a lot of different things, it's a god function), so I've been gradually breaking it up into smaller pieces / functions whenever I can. -
registerPlugins
signature could be the same, as it's derived fromplugins
. (It already has a large signature with some weird stuff ala opts + options?? LOL
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.
Done
a00272a
to
175d6f6
Compare
175d6f6
to
7a39c69
Compare
@zikaari Looks good! CI is failing =) |
70adbf6
to
612d815
Compare
This pull request is the other half of segmentio/action-destinations#1276.
This patch just adds the necessary moving parts that will let users bring in action-destinations from NPM.