-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: Allow skip initialization on meta #2037
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new property, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FP as FacebookPixel
participant FBQ as fbq
User->>FP: Instantiate with skipInitialization flag
alt skipInitialization is false
FP->>FBQ: Initialize pixel using user payload
else skipInitialization is true
FP-->>User: Skip initialization
end
FP->>FP: Execute isReady check (verify fbq pixel ID exists)
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js (1)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js (2)
118-119
: Add safer null checking for getState().The current implementation might throw if
getState()
returns undefined.Apply this diff to add safer null checking:
- const isInitialized = (window.fbq?.getState?.().pixels || []).some(p => p.id === this.pixelId); + const isInitialized = (window.fbq?.getState?.()?.pixels || []).some(p => p.id === this.pixelId); return this.isLoaded() && isInitialized;
117-120
: Consider enhancing initialization verification.While the
isReady
check verifies FB initialization, it doesn't guarantee that HubSpot will eventually initialize FB. Consider:
- Adding a timeout mechanism to prevent indefinite waiting
- Implementing a fallback initialization if HubSpot fails to initialize FB
- Adding logging to help debug initialization issues
This will make the integration more robust and easier to troubleshoot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js
(3 hunks)
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js
Outdated
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js
Outdated
Show resolved
Hide resolved
7da20c0
to
6ad55ef
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js
(4 hunks)
🔇 Additional comments (2)
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js (2)
51-51
: LGTM!The
skipInitialization
property is correctly added and follows the established pattern for configuration properties.
79-82
: LGTM! The initialization skip logic is well-implemented.The code correctly loads the FB SDK while skipping the initialization, allowing external tools like HubSpot to handle the initialization process.
packages/analytics-js-integrations/src/integrations/FacebookPixel/browser.js
Show resolved
Hide resolved
6ad55ef
to
9b6d3c8
Compare
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
PR Description
Some tools like hubspot tracking initialize meta with their own external id.
To avoid the double initialization we want to delegate fb initialization to hubspot.
On this PR we aim to conditionally disbale meta intialization through rudderstack.
Summary by CodeRabbit