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

Fix/Handle duplicate VASTAdTagURI in AdPod #452

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

Rapha0511
Copy link
Contributor

@Rapha0511 Rapha0511 commented Jul 19, 2023

Description

the parentURLs array was used to store and check if a VASTAdTagURI has been called before, to prevent multiple calls to the same URL. This array has been removed as it is no longer necessary.

Now, wrappers within an AdPod can reference the same VASTAdTagURI. This change is based on the consideration that, in case where a wrapper is calling itself, the fetching process will stop after the maxWrapperDepth is reached. This mechanism prevents infinite loops.

Issue

#450

Type

  • Breaking change
  • Enhancement
  • Fix
  • Documentation
  • Tooling

Copy link
Contributor

@ZacharieTFR ZacharieTFR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! 🎉 Just left a small comment about unit tests

Comment on lines 89 to 90
done();
expect(urlHandlerSpy).toHaveBeenCalledWith(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to put the done after the expect as Jest do in their documentation https://jestjs.io/docs/asynchronous#callbacks

Copy link
Contributor

@ZacharieTFR ZacharieTFR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! 👍

@ZacharieTFR ZacharieTFR merged commit 3919e04 into master Aug 23, 2023
1 check passed
@ZacharieTFR ZacharieTFR deleted the fix/duplicate-vasttaguri-in-adpod branch August 23, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants