Change package.json main entry

[germain-gg]

Hi,

This library is intended to be used client-side and NPM recommends to use the browser field instead of main in that specific case.

https://docs.npmjs.com/files/package.json#browser

Not 100% sure if that could have an impact on the build/release pipeline. So far my testing seemed fine.

[PTaylour]

it probably won't cause the build/release pipeline to fail—but there isn't any test coverage for how package.json is interpreted.

This test fails:

/**
 * this is a quick and dirty test to ensure that our package.json fields point to a built file
 */

const PATH_TO_PACKAGE_JSON = "../../";

test("require(rootDir) probably points to a file", () => {
    const load = () => require(PATH_TO_PACKAGE_JSON);
    expect(load).not.toThrowError(/Cannot find module/);
});

So would we also expect webpack projects to fail?

[germain-gg]

Webpack supports the browser field by default, https://webpack.js.org/configuration/resolve/#resolve-mainfields

I'll look into fixing the tests.

[PTaylour]

Cool. Looks like rollup etc all support the browser field too.

That test isn't in the codebase at the moment (and is pretty flawed as importing videocontext in node crashes due to lack of window), it was just me thinking of how to test this.

[germain-gg]

Fair enough,

I think that's a completely valid point. I believe there is a way to configure jest so that it mocks a browser environment using JSDOM.

I might give that a go to strengthen the test env.

On Fri, 5 Oct 2018, 17:42 Pete Taylour, notifications@github.com wrote:

Cool. Looks like rollup etc all support the browser field too.

That test isn't in the codebase at the moment (and is pretty flawed as importing videocontext in node crashes due to lack of window), it was just me thinking of how to test this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bbc/VideoContext/pull/110#issuecomment-427427599, or mute the thread https://github.com/notifications/unsubscribe-auth/AAu_TzoYF_CzilY4mUdA0TYcP3dW_4Lfks5uh4wHgaJpZM4XE7zN .

[PTaylour]

@gsouquet shall I merge this now and open a ticket for the test?

[germain-gg]

@PTaylour you mention that the tests are failing due to the window object. However when looking at the Jest documentation, it states that the testEnvironment option uses jsdom by defaut. https://jestjs.io/docs/en/configuration.html#testenvironment-string

It should implement a mock window object. However it does not implement non-DOM API.

I'm happy to create another GitHub issue explaining the problem. Will investigate this further in the future.

[PTaylour]

@gsouquet interesting, maybe the error was pointing at something else.

Yes an issue would be great.

I'm going to hold over making a fresh release until tomorrow morning incase for whatever reason it does need rolling back