Navigation Menu

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

Add jquery-migrate to Modal test #1599

Merged
merged 1 commit into from Nov 19, 2019

Conversation

jbruni
Copy link
Contributor

@jbruni jbruni commented Nov 15, 2019

What?

As written at assets/js/theme/global/jquery-migrate.js:

// Needed because we use Foundation 5.5, which expects jQuery 2.x. However,
// rather than bringing in all of jquery-migrate, we're cherry picking individual
// fixes needed for Foundation.

This is also true for the testing environment. Foundation 5.5 still expects jQuery 2.x, and the need for jquery-migrate is the same.

However, Foundation is used at assets/js/test-unit/theme/global/modal.spec.js, but jquery-migrate is not loaded.

The purpose of this PR is to fix this situation. It includes the missing jquery-migrate import to this test, before Foundation is used.

Why this needs to be fixed? (easy to reproduce)

We stumbled upon this problem when adding our own tests to our custom theme. It is very easy to reproduce.

➡️ Simply save the following test specification as
assets/js/test-unit/theme/common/sample.spec.js:

import '../../../theme/global/jquery-migrate';
import $ from 'jquery';
import foundation from '../../../theme/global/foundation';

describe('Sample', () => {
    beforeEach(() => {
        window.Foundation.global.namespace = '';
        foundation($(document));
    });

    describe('when nothing happens', () => {
        it('true should be true', () => {
            expect(true).toBeTruthy();
        });
    });
});

➡️ Now, run npx karma start (or npx grunt karma) to run the tests. It will fail.

Why does it fail?

When we call foundation($('document'));, it internally calls $(window).load() - but the load event has been removed in newer jQuery versions. The $(window).load() Foundation code would work with old jQuery (or modified by jquery-migrate), but in modern jQuery it will call the Ajax load function instead!

Even though we are importing jquery-migrate, the Modal test ran before, and Foundation has been somehow tied to a jQuery instance which does not have the jquery-migrate changes.

➡️ Now, apply the simple change from this PR, and run the tests again. It will succeed!

Why does it suceed?

When Foundation is initialized at Modal testing, jquery-migrate changes were already applied, and thus Foundation gets tied to a jQuery instance properly modified, and works as expected at all the following tests.


In fact, a separate test specification is not required to reproduce the issue. We can just add a few lines to the beginning of the Modal test spec, and it will also reveal the problem.

Screenshots

⬇️ Running the test which uses Foundation, before this PR fix:

test-failure

⬇️ Running the test which uses Foundation, after this PR fix:

test-success

@bigbot
Copy link

bigbot commented Nov 15, 2019

Autotagging @bigcommerce/storefront-team @davidchin

@bookernath bookernath merged commit 741694e into bigcommerce:master Nov 19, 2019
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

3 participants