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 Empty adSlot Object Caused by Ad Blocker #21

Merged

Conversation

blakemcintyre
Copy link
Contributor

Issue

When using an ad blocker adSlot can become an empty object which causes the call to adSlot.getServices to produce the error: "getServices is not a function".

Solution

By adding the check adSlot.hasOwnProperty("getServices") we can ensure that the adSlot object has the correct function before attempting to call it.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2017

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Mar 21, 2017

Codecov Report

Merging #21 into master will decrease coverage by 1.04%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
- Coverage   93.98%   92.93%   -1.05%     
==========================================
  Files           7        7              
  Lines         565      566       +1     
==========================================
- Hits          531      526       -5     
- Misses         34       40       +6
Impacted Files Coverage Δ
src/Bling.js 88.32% <100%> (-1.53%) ⬇️
src/createManager.js 92.16% <25%> (-1.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2d0e33...b640665. Read the comment docs.

Issue

When using an ad blocker adSlot can become an empty object which
causes the call to adSlot.getServices to produce the error:
"getServices is not a function".

Solution

By adding the check adSlot.hasOwnProperty("getServices") we can ensure
that the adSlot object has the correct function before attempting to
call it.
@blakemcintyre blakemcintyre force-pushed the fix-empty-adSlot-object-caused-by-ad-blocker branch from c69288b to 81c157b Compare March 22, 2017 18:02
@potench
Copy link
Contributor

potench commented Mar 22, 2017

I've been trying to think of a way to simulate this AdBlocker issue in a unit test to verify your PR, but not having any luck. A test like this would be nice to add to this PR, any thoughts on how to achieve it?

    it("fails gracefully with adBlocker present", (done) => {
        Bling.once(Events.RENDER, () => {
            delete instance.adSlot.getServices; // simulate adblocker
            expect(() => {
                instance.render();
            }).to.not.throw("adSlot.getServices is not a function");
            expect(() => {
                instance.clear();
            }).to.not.throw("adSlot.getServices is not a function");
            done();
        });

        const instance = ReactTestUtils.renderIntoDocument(
            <Bling
                adUnitPath="/4595/nfl.test.open"
                slotSize={[300, 250]}
            />
        );
    });

@blakemcintyre
Copy link
Contributor Author

@potench I tried your test and it passed, it also passed when I removed my change. I did some digging, I'm new to this code base, and found that when the tests are run there is never a case where _adSlot has getServices. getServices is listed in src/utils/apiList.js and is from google and not part of the actual code base. I'm at a bit of a loss has how to fully test this with rendering Bling.

@blakemcintyre
Copy link
Contributor Author

Would it be possible to get this PR merged in quickly? I only ask because we are using react-gpt on http://www.thescoreesports.com/lol/scores and when you use an ad blocker it prevents the site from functioning correctly due to the exception that is thrown. You can also see the error in the in the console when running the example http://localhost:8080/single-request/index.html. If there is anything else you need for this let me know and I will update the PR.

In an attempt to work around this issue we've looked at using my branch for the dependency but that ran into a couple of issues with dist and lib directories not being included in the repo. In order to be able to use it we would need to manually add those missing directories which isn't an ideal case.

@potench potench merged commit 7f9a989 into nfl:master Mar 23, 2017
@potench
Copy link
Contributor

potench commented Mar 23, 2017

version 0.2.4 was published with your PR

Sorry for delay; your commit caused codecoverage to drop below threshold indicating that we might need some more tests around the clear() and renderAll() methods for future use.

Thanks for the PR

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.

None yet

4 participants