-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add GTM integration #814
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
Open
bydawen
wants to merge
1
commit into
openedx:master
Choose a base branch
from
raccoongang:rg/gtm-support-mfe
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/** | ||
* @implements {GoogleTagManagerLoader} | ||
* @memberof module:GoogleTagManagerLoader | ||
*/ | ||
class GoogleTagManagerLoader { | ||
constructor({ config }) { | ||
this.gtmId = config.GOOGLE_TAG_MANAGER_ID; | ||
} | ||
|
||
loadScript() { | ||
if (!this.gtmId) { | ||
return; | ||
} | ||
|
||
global.google_tag_manager = global.google_tag_manager || []; | ||
const { google_tag_manager: googleTagManager } = global; | ||
|
||
// If the snippet was invoked do nothing. | ||
if (googleTagManager.invoked) { | ||
return; | ||
} | ||
|
||
// Invoked flag, to make sure the snippet | ||
// is never invoked twice. | ||
googleTagManager.invoked = true; | ||
|
||
googleTagManager.load = (id) => { | ||
const gtmScript = document.createElement('script'); | ||
gtmScript.type = 'text/javascript'; | ||
gtmScript.innerHTML = ` | ||
(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start': | ||
new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0], | ||
j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src= | ||
'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f); | ||
})(window,document,'script','dataLayer', '${id}'); | ||
`; | ||
|
||
// Insert our scripts next to the first script element. | ||
const first = document.getElementsByTagName('script')[0]; | ||
first.parentNode.insertBefore(gtmScript, first); | ||
}; | ||
|
||
// Load gtmAnalytics. | ||
googleTagManager.load(this.gtmId); | ||
} | ||
} | ||
|
||
export default GoogleTagManagerLoader; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import GoogleTagManagerLoader from './GoogleTagManagerLoader'; | ||
|
||
describe('GoogleTagManagerLoader', () => { | ||
const mockGTMId = 'GOOGLE_TAG_MANAGER_ID_test_id'; | ||
let insertBeforeMock; | ||
|
||
beforeEach(() => { | ||
global.google_tag_manager = undefined; | ||
insertBeforeMock = jest.fn(); | ||
|
||
document.getElementsByTagName = jest.fn(() => [ | ||
{ | ||
parentNode: { | ||
insertBefore: insertBeforeMock, | ||
}, | ||
}, | ||
]); | ||
}); | ||
|
||
it('should load GTM script', () => { | ||
const loader = new GoogleTagManagerLoader({ config: { GOOGLE_TAG_MANAGER_ID: mockGTMId } }); | ||
|
||
loader.loadScript(); | ||
|
||
expect(global.google_tag_manager).toBeDefined(); | ||
expect(global.google_tag_manager.invoked).toBe(true); | ||
|
||
const firstScript = document.getElementsByTagName()[0]; | ||
expect(firstScript.parentNode.insertBefore).toHaveBeenCalled(); | ||
}); | ||
|
||
it('should not load script if account is not defined', () => { | ||
const loader = new GoogleTagManagerLoader({ config: { GOOGLE_TAG_MANAGER_ID: '' } }); | ||
|
||
loader.loadScript(); | ||
|
||
expect(global.google_tag_manager).toBeUndefined(); | ||
}); | ||
|
||
it('should not load script if google_tag_manager is already invoked', () => { | ||
global.google_tag_manager = { invoked: true }; | ||
const loader = new GoogleTagManagerLoader({ config: { GOOGLE_TAG_MANAGER_ID: mockGTMId } }); | ||
|
||
loader.loadScript(); | ||
|
||
expect(global.google_tag_manager.invoked).toBe(true); | ||
expect(document.getElementsByTagName).not.toHaveBeenCalled(); | ||
}); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
/* eslint-disable import/prefer-default-export */ | ||
export { default as GoogleAnalyticsLoader } from './GoogleAnalyticsLoader'; | ||
export { default as GoogleTagManagerLoader } from './GoogleTagManagerLoader'; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since we have
GoogleAnalyticsLoader
defined here already, it looks like the right place to add support for something like GTM. However, adding support for arbitrary external scripts, however popular, is not really maintainable for the project.Instead, it would be better if we made
externalScripts
configurable viaenv.config.jsx
, removing Google Analytics support in the process but allowing operators to add any external script they need without requiring the platform to explicitly support it. Is this a change you're comfortable contributing?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.
Hello @arbrandes, thank you for the idea of making
externalScripts
configurable!We'll discuss this approach with our team and may consider creating a separate PR to explore that direction.
For now, I think to keep the current implementation as-is, since in our projects we frequently use GTM in this way. We’d also like to gather more feedback from the community, including real-world examples of GTM usage, to help refine and potentially improve our solution.
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.
@arbrandes thanks for your feedback! Can we go ahead with the changes we have in this PR and create a separate issue to implement the functionality of adding
externalScripts
viaenv.config.jsx
?