Skip to content
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

chore: use provided tag insertion method #2

Merged
merged 3 commits into from
May 3, 2023

Conversation

naripok
Copy link

@naripok naripok commented Mar 29, 2023

Summary

Closes https://github.com/AirGrid/rmap/issues/1459

Type of change

  • Bugfix

Description of change

Use loadExternalScript instead of custom code for tag module loading

Other information

@ydennisy

@naripok naripok self-assigned this Mar 29, 2023
@naripok naripok requested a review from ydennisy March 29, 2023 19:12
@naripok naripok marked this pull request as draft March 29, 2023 19:20
@naripok
Copy link
Author

naripok commented Mar 29, 2023

@ydennisy I did what they asked, but we have a problem which I'm not sure how to tackle yet:
image

EDIT:
Well, I found this in their tests, and I think it may pose a problem with how we interpolate the module url for the publishers...

    it('only allows whitelisted vendors to load scripts', function () {
      adLoader.loadExternalScript('someURL', 'criteo');
      expect(utilsLogErrorStub.called).to.be.false;
      expect(utilsinsertElementStub.called).to.be.true;
    });

If we need a fixed url to be whitelisted, then we're gonna need to whitelist all the possible publisher specific urls, and update the whitelist (wherever it is, haven't found yet) every time we add a new publisher... :(

EDIT:
Hmm, the docs look to imply that we need a more lengthy process in order to be able to load our module...
It involves asking for a prebid's maintainer to clone this repo, adding code there, then loading from it in the adapter. If I understood correctly, obviously...

@naripok naripok force-pushed the chore/1459-use-tag-insertion-method branch from 9c94fd2 to ced5644 Compare March 29, 2023 19:28
@naripok naripok force-pushed the chore/1459-use-tag-insertion-method branch from ced5644 to a885439 Compare March 29, 2023 19:31
@naripok
Copy link
Author

naripok commented Mar 29, 2023

Another consideration, @ydennisy .
Prebid's loadExternalScript accepts an attributes parameter, which it sets to the script being loaded.
I've tried moving our accountId, publisherId and apiKey to it, but it won't set the attributes in the same fashion we do now, and it breaks our tests.
As I'm not sure of the implications of it, I'm leaving the attribute setting as is, now done in the callback function called as continuation in the loadExternalScript function.

@naripok naripok marked this pull request as ready for review May 3, 2023 18:51
@naripok naripok merged commit d5e12e6 into master May 3, 2023
@naripok naripok deleted the chore/1459-use-tag-insertion-method branch May 3, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant