-
Notifications
You must be signed in to change notification settings - Fork 254
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
Standalone and independently versioned browser-destinations #1276
Conversation
515288e
to
00f31ef
Compare
1c2a94e
to
faefba6
Compare
Hi @zikaari , I'm more than happy to move to a convo on slack, but tldr; actions team was hoping to deprecate the cli very soon (2 weeks perhaps) in favor of the private action-cli (for security reasons). Would you be able to make these changes in the action-cli repo instead? |
@zikaari Can we have documentation on the new steps to publish an action destinations to npm? |
@silesky That intricacies of that process are yet to be finalized and will be discussed once the PR is greenlit |
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.
Looks good to me - in particular functionality with browser action bundles/versioning still looks good. I think getting sign off from someone on the action destinations team would be good too.
This looks really awesome! I think this is a huge improvement. I only have one suggestion/concern around naming convention -- I saw the name format "@segment/actions-[pluginName]" -- would it make sense to be "@segment/analytics-destination-[pluginName]" (I don't feel strongly, but it seems like using 'analytics' might be helpful for SEO / discoverability, since this is tightly coupled to analytics.js. For example -- for consent, we do "@segment/analytics-consent-onetrust" This would also be kind of consistent with classic, where we 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.
Thanks for working with us and testing this! Appreciate the meeting walking us through ✅
@@ -22,16 +22,6 @@ | |||
"main": "dist/cjs/index.js", | |||
"module": "dist/esm/index.js", | |||
"types": "dist/cjs/index.d.ts", | |||
"exports": { |
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.
why is this here?
This pull requests makes each browser destinations accessible through NPM by making it standalone and independently versioned. This will let customers install the destinations from NPM and have it packaged into their main app bundle through webpack or other bundlers, thus eliminating the reliance on CDN and a lot of concurrent network requests.
This pull request is multi-stage process, each of which is a separate commit so that while reviewing you're not being blasted with all the changes at once thus overwhelming you too much. It is advised that you use the commit filter in to isolate and audit one commit at a time.
Most of the existing functionality and behaviors work with the exception of the following:
build-cjs
script has been removed, unless there's an explicit need for it, the output is not warranted